| Message ID | 1582864911-30823-1-git-send-email-xuyang2018.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] KEYS: reaching the keys quotas correctly | expand |
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
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
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 > >
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
How about: keys: Fix the keys quotas limit check It's a bit unfortunate that "key" is also usable as an adjective. David
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
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
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
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
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;
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(-)