[v3] KEYS: reaching the keys quotas correctly
diff mbox series

Message ID 1582864911-30823-1-git-send-email-xuyang2018.jy@cn.fujitsu.com
State New
Headers show
Series
  • [v3] KEYS: reaching the keys quotas correctly
Related show

Commit Message

Yang Xu Feb. 28, 2020, 4:41 a.m. UTC
Currently, when we add a new user key, the calltrace as below:

add_key()
  key_create_or_update()
    key_alloc()
    __key_instantiate_and_link
      generic_key_instantiate
        key_payload_reserve
          ......

Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
we can reach max bytes/keys in key_alloc, but we forget to remove this
limit when we reserver space for payload in key_payload_reserve. So we
can only reach max keys but not max bytes when having delta between plen
and type->def_datalen. Remove this limit when instantiating the key, so we
can keep consistent with key_alloc.

Also, fix the similar problem in keyctl_chown_key().

Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 security/keys/key.c    | 2 +-
 security/keys/keyctl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Jarkko Sakkinen March 2, 2020, 12:52 p.m. UTC | #1
On Fri, 2020-02-28 at 12:41 +0800, Yang Xu wrote:
> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Also, fix the similar problem in keyctl_chown_key().
> 
> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
Eric Biggers March 3, 2020, 4:17 a.m. UTC | #2
On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
>
> Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly

The subject should be in imperative tense, like
"KEYS: reach the keys quotas correctly"

> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Also, fix the similar problem in keyctl_chown_key().
> 
> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

Otherwise this looks fine.  Thanks!

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
Yang Xu March 3, 2020, 4:33 a.m. UTC | #3
on 2020/03/03 12:17, Eric Biggers wrote:
> On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
>>
>> Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly
> 
> The subject should be in imperative tense, like
> "KEYS: reach the keys quotas correctly"
Sound reasonable, I think maintainer can change this when merged this patch.

Best Regards
Yang Xu
> 
>> Currently, when we add a new user key, the calltrace as below:
>>
>> add_key()
>>    key_create_or_update()
>>      key_alloc()
>>      __key_instantiate_and_link
>>        generic_key_instantiate
>>          key_payload_reserve
>>            ......
>>
>> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
>> we can reach max bytes/keys in key_alloc, but we forget to remove this
>> limit when we reserver space for payload in key_payload_reserve. So we
>> can only reach max keys but not max bytes when having delta between plen
>> and type->def_datalen. Remove this limit when instantiating the key, so we
>> can keep consistent with key_alloc.
>>
>> Also, fix the similar problem in keyctl_chown_key().
>>
>> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
>> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
>> Cc: Eric Biggers <ebiggers@google.com>
>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> 
> Otherwise this looks fine.  Thanks!
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> 
> - Eric
> 
>
Jarkko Sakkinen March 3, 2020, 8:18 p.m. UTC | #4
On Mon, Mar 02, 2020 at 08:17:32PM -0800, Eric Biggers wrote:
> On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
> >
> > Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly
> 
> The subject should be in imperative tense, like
> "KEYS: reach the keys quotas correctly"

Preferably with a capital letter .

> 
> > Currently, when we add a new user key, the calltrace as below:
> > 
> > add_key()
> >   key_create_or_update()
> >     key_alloc()
> >     __key_instantiate_and_link
> >       generic_key_instantiate
> >         key_payload_reserve
> >           ......
> > 
> > Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> > we can reach max bytes/keys in key_alloc, but we forget to remove this
> > limit when we reserver space for payload in key_payload_reserve. So we
> > can only reach max keys but not max bytes when having delta between plen
> > and type->def_datalen. Remove this limit when instantiating the key, so we
> > can keep consistent with key_alloc.
> > 
> > Also, fix the similar problem in keyctl_chown_key().
> > 
> > Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> > Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> > Cc: Eric Biggers <ebiggers@google.com>
> > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> 
> Otherwise this looks fine.  Thanks!
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

David, should I pick this is up to my tree?

/Jarkko
David Howells March 19, 2020, 3:08 p.m. UTC | #5
How about:

	keys: Fix the keys quotas limit check

It's a bit unfortunate that "key" is also usable as an adjective.

David
Jarkko Sakkinen March 19, 2020, 9:15 p.m. UTC | #6
On Thu, Mar 19, 2020 at 03:08:21PM +0000, David Howells wrote:
> How about:
> 
> 	keys: Fix the keys quotas limit check
> 
> It's a bit unfortunate that "key" is also usable as an adjective.
> 
> David

Unfortunately it is already hanging here:

https://www.lkml.org/lkml/2020/3/15/314

/Jarkko
David Howells March 19, 2020, 9:30 p.m. UTC | #7
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> Unfortunately it is already hanging here:
> 
> https://www.lkml.org/lkml/2020/3/15/314

Hanging? Or queued?

David
Jarkko Sakkinen March 20, 2020, 1:45 a.m. UTC | #8
On Thu, Mar 19, 2020 at 09:30:13PM +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > Unfortunately it is already hanging here:
> > 
> > https://www.lkml.org/lkml/2020/3/15/314
> 
> Hanging? Or queued?

Not yet queued.

Should I request to withdraw it? There is still time to do that.

/Jarkko
David Howells March 20, 2020, 8:59 a.m. UTC | #9
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> > > Unfortunately it is already hanging here:
> > > 
> > > https://www.lkml.org/lkml/2020/3/15/314
> > 
> > Hanging? Or queued?
> 
> Not yet queued.
> 
> Should I request to withdraw it? There is still time to do that.

I was making up a series of fix patches to send to Linus.  If you want to send
your queue instead, you can stick my Acked-by on this patch.

David

Patch
diff mbox series

diff --git a/security/keys/key.c b/security/keys/key.c
index 718bf7217420..e959b3c96b48 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -382,7 +382,7 @@  int key_payload_reserve(struct key *key, size_t datalen)
 		spin_lock(&key->user->lock);
 
 		if (delta > 0 &&
-		    (key->user->qnbytes + delta >= maxbytes ||
+		    (key->user->qnbytes + delta > maxbytes ||
 		     key->user->qnbytes + delta < key->user->qnbytes)) {
 			ret = -EDQUOT;
 		}
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..d1a3dea58dee 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -937,8 +937,8 @@  long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 				key_quota_root_maxbytes : key_quota_maxbytes;
 
 			spin_lock(&newowner->lock);
-			if (newowner->qnkeys + 1 >= maxkeys ||
-			    newowner->qnbytes + key->quotalen >= maxbytes ||
+			if (newowner->qnkeys + 1 > maxkeys ||
+			    newowner->qnbytes + key->quotalen > maxbytes ||
 			    newowner->qnbytes + key->quotalen <
 			    newowner->qnbytes)
 				goto quota_overrun;