diff mbox series

tomoyo: Check exceeded quota early in tomoyo_domain_quota_is_ok().

Message ID 20211214094526.2442154-1-dvyukov@google.com (mailing list archive)
State New, archived
Headers show
Series tomoyo: Check exceeded quota early in tomoyo_domain_quota_is_ok(). | expand

Commit Message

Dmitry Vyukov Dec. 14, 2021, 9:45 a.m. UTC 
If tomoyo is used in a testing/fuzzing environment in learning mode,
for lots of domains the quota will be exceeded and stay exceeded
for prolonged periods of time. In such cases it's pointless (and slow)
to walk the whole acl list again and again just to rediscover that
the quota is exceeded. We already have the TOMOYO_DIF_QUOTA_WARNED flag
that notes the overflow condition. Check it early to avoid the slowdown.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 security/tomoyo/util.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


base-commit: 5472f14a37421d1bca3dddf33cabd3bd6dbefbbc

Comments

Tetsuo Handa Dec. 14, 2021, 11:36 a.m. UTC | #1 
On 2021/12/14 18:45, Dmitry Vyukov wrote:
> If tomoyo is used in a testing/fuzzing environment in learning mode,
> for lots of domains the quota will be exceeded and stay exceeded
> for prolonged periods of time. In such cases it's pointless (and slow)
> to walk the whole acl list again and again just to rediscover that
> the quota is exceeded. We already have the TOMOYO_DIF_QUOTA_WARNED flag
> that notes the overflow condition. Check it early to avoid the slowdown.

Thank you.

This patch will make a slight but user visible change.

When tomoyo_profile(domain->ns, domain->profile)->pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] is
increased (or domain->profile switches to a different profile which has larger
pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value) after domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true
is called, tomoyo_domain_quota_is_ok() will continue returning "false", and ACLs are no longer
appended.

Therefore, administrator will have to manually do domain->flags[TOMOYO_DIF_QUOTA_WARNED] = false
after increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile).

But since the message

  WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.

is a hint that tells administrator that "you will surely fail to try the enforcing mode on this
domain because the kernel has failed to automatically append at least one ACL to this domain",
administrator would have to retry the learning mode after increasing
pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) even without this patch.

Therefore, asking administrator to also clear domain->flags[TOMOYO_DIF_QUOTA_WARNED] after
increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) would be
tolerable...
Dmitry Vyukov Dec. 14, 2021, 11:42 a.m. UTC | #2 
On Tue, 14 Dec 2021 at 12:36, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2021/12/14 18:45, Dmitry Vyukov wrote:
> > If tomoyo is used in a testing/fuzzing environment in learning mode,
> > for lots of domains the quota will be exceeded and stay exceeded
> > for prolonged periods of time. In such cases it's pointless (and slow)
> > to walk the whole acl list again and again just to rediscover that
> > the quota is exceeded. We already have the TOMOYO_DIF_QUOTA_WARNED flag
> > that notes the overflow condition. Check it early to avoid the slowdown.
>
> Thank you.
>
> This patch will make a slight but user visible change.
>
> When tomoyo_profile(domain->ns, domain->profile)->pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] is
> increased (or domain->profile switches to a different profile which has larger
> pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value) after domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true
> is called, tomoyo_domain_quota_is_ok() will continue returning "false", and ACLs are no longer
> appended.
>
> Therefore, administrator will have to manually do domain->flags[TOMOYO_DIF_QUOTA_WARNED] = false
> after increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile).
>
> But since the message
>
>   WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.
>
> is a hint that tells administrator that "you will surely fail to try the enforcing mode on this
> domain because the kernel has failed to automatically append at least one ACL to this domain",
> administrator would have to retry the learning mode after increasing
> pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) even without this patch.
>
> Therefore, asking administrator to also clear domain->flags[TOMOYO_DIF_QUOTA_WARNED] after
> increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) would be
> tolerable...

Should we reset flags[TOMOYO_DIF_QUOTA_WARNED] on any writes that
change TOMOYO_PREF_MAX_LEARNING_ENTRY?

If I am increasing TOMOYO_PREF_MAX_LEARNING_ENTRY because I observed
the warning, it's useful for me to receive the warning again.
Tetsuo Handa Dec. 15, 2021, 11:46 a.m. UTC | #3 
On 2021/12/14 20:42, Dmitry Vyukov wrote:
>> Therefore, asking administrator to also clear domain->flags[TOMOYO_DIF_QUOTA_WARNED] after
>> increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) would be
>> tolerable...
> 
> Should we reset flags[TOMOYO_DIF_QUOTA_WARNED] on any writes that
> change TOMOYO_PREF_MAX_LEARNING_ENTRY?
> 
> If I am increasing TOMOYO_PREF_MAX_LEARNING_ENTRY because I observed
> the warning, it's useful for me to receive the warning again.

I decided not to reset flags[TOMOYO_DIF_QUOTA_WARNED] automatically, and
applied your proposal as-is. Thank you.

https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits/04e57a2d952bbd34bc45744e72be3eecdc344294
Dmitry Vyukov Dec. 15, 2021, 1:17 p.m. UTC | #4 
On Wed, 15 Dec 2021 at 12:46, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2021/12/14 20:42, Dmitry Vyukov wrote:
> >> Therefore, asking administrator to also clear domain->flags[TOMOYO_DIF_QUOTA_WARNED] after
> >> increasing pref[TOMOYO_PREF_MAX_LEARNING_ENTRY] value (or changing domain->profile) would be
> >> tolerable...
> >
> > Should we reset flags[TOMOYO_DIF_QUOTA_WARNED] on any writes that
> > change TOMOYO_PREF_MAX_LEARNING_ENTRY?
> >
> > If I am increasing TOMOYO_PREF_MAX_LEARNING_ENTRY because I observed
> > the warning, it's useful for me to receive the warning again.
>
> I decided not to reset flags[TOMOYO_DIF_QUOTA_WARNED] automatically, and
> applied your proposal as-is. Thank you.
>
> https://osdn.net/projects/tomoyo/scm/git/tomoyo-test1/commits/04e57a2d952bbd34bc45744e72be3eecdc344294

Thanks!

It will appear in v5.17, right?
diff mbox series

Patch

diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 1da2e3722b126..af8cd2af3466d 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -1051,6 +1051,8 @@  bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
 		return false;
 	if (!domain)
 		return true;
+	if (READ_ONCE(domain->flags[TOMOYO_DIF_QUOTA_WARNED]))
+		return false;
 	list_for_each_entry_rcu(ptr, &domain->acl_info_list, list,
 				srcu_read_lock_held(&tomoyo_ss)) {
 		u16 perm;
@@ -1096,14 +1098,12 @@  bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
 	if (count < tomoyo_profile(domain->ns, domain->profile)->
 	    pref[TOMOYO_PREF_MAX_LEARNING_ENTRY])
 		return true;
-	if (!domain->flags[TOMOYO_DIF_QUOTA_WARNED]) {
-		domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true;
-		/* r->granted = false; */
-		tomoyo_write_log(r, "%s", tomoyo_dif[TOMOYO_DIF_QUOTA_WARNED]);
+	WRITE_ONCE(domain->flags[TOMOYO_DIF_QUOTA_WARNED], true);
+	/* r->granted = false; */
+	tomoyo_write_log(r, "%s", tomoyo_dif[TOMOYO_DIF_QUOTA_WARNED]);
 #ifndef CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING
-		pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
-			domain->domainname->name);
+	pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
+		domain->domainname->name);
 #endif
-	}
 	return false;
 }