Once a key's reference count has been reduced to 0, the garbage collector thread may destroy it at any time and so key_put() is not allowed to touch the key after that point. The most key_put() is normally allowed to do is to touch key_gc_work as that's a static global variable.
However, in an effort to speed up the reclamation of quota, this is now done in key_put() once the key's usage is reduced to 0 - but now the code is looking at the key after the deadline, which is forbidden.
Fix this by using a flag to indicate that a key can be gc'd now rather than looking at the key's refcount in the garbage collector.
Fixes: 9578e327b2b4 ("keys: update key quotas in key_put()") Reported-by: syzbot+6105ffc1ded71d194d6d@syzkaller.appspotmail.com Signed-off-by: David Howells dhowells@redhat.com Tested-by: syzbot+6105ffc1ded71d194d6d@syzkaller.appspotmail.com cc: Jarkko Sakkinen jarkko@kernel.org cc: Oleg Nesterov oleg@redhat.com cc: Kees Cook kees@kernel.org cc: Hillf Danton hdanton@sina.com, cc: keyrings@vger.kernel.org Cc: stable@vger.kernel.org # v6.10+ --- include/linux/key.h | 1 + security/keys/gc.c | 4 +++- security/keys/key.c | 2 ++ 3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/key.h b/include/linux/key.h index 074dca3222b9..ba05de8579ec 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -236,6 +236,7 @@ struct key { #define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */ #define KEY_FLAG_KEEP 8 /* set if key should not be removed */ #define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */ +#define KEY_FLAG_FINAL_PUT 10 /* set if final put has happened on key */
/* the key type and key description string * - the desc is used to match a key against search criteria diff --git a/security/keys/gc.c b/security/keys/gc.c index 7d687b0962b1..f27223ea4578 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -218,8 +218,10 @@ static void key_garbage_collector(struct work_struct *work) key = rb_entry(cursor, struct key, serial_node); cursor = rb_next(cursor);
- if (refcount_read(&key->usage) == 0) + if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) { + smp_mb(); /* Clobber key->user after FINAL_PUT seen. */ goto found_unreferenced_key; + }
if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) { if (key->type == key_gc_dead_keytype) { diff --git a/security/keys/key.c b/security/keys/key.c index 3d7d185019d3..7198cd2ac3a3 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -658,6 +658,8 @@ void key_put(struct key *key) key->user->qnbytes -= key->quotalen; spin_unlock_irqrestore(&key->user->lock, flags); } + smp_mb(); /* key->user before FINAL_PUT set. */ + set_bit(KEY_FLAG_FINAL_PUT, &key->flags); schedule_work(&key_gc_work); } }
On 03/19, David Howells wrote:
--- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -218,8 +218,10 @@ static void key_garbage_collector(struct work_struct *work) key = rb_entry(cursor, struct key, serial_node); cursor = rb_next(cursor);
if (refcount_read(&key->usage) == 0)
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */ goto found_unreferenced_key;}if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) { if (key->type == key_gc_dead_keytype) { diff --git a/security/keys/key.c b/security/keys/key.c index 3d7d185019d3..7198cd2ac3a3 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -658,6 +658,8 @@ void key_put(struct key *key) key->user->qnbytes -= key->quotalen; spin_unlock_irqrestore(&key->user->lock, flags); }
smp_mb(); /* key->user before FINAL_PUT set. */
Can't resist, smp_mb__before_atomic() should equally work, but this doesn't really matter, please forget.
set_bit(KEY_FLAG_FINAL_PUT, &key->flags); schedule_work(&key_gc_work);
I believe this patch is correct,
Reviewed-by: Oleg Nesterov oleg@redhat.com
On Wed, 19 Mar 2025 at 09:31, Oleg Nesterov oleg@redhat.com wrote:
Can't resist, smp_mb__before_atomic() should equally work, but this doesn't really matter, please forget.
We really should have "test_bit_acquire()" and "set_bit_release()".
Well, we do have the test_bit_acquire().
We just don't have the set_bit side, because we only have the bit clearing version (and it's called "clear_bit_unlock()" for historical reasons).
Annoying.
Linus
Linus Torvalds torvalds@linuxfoundation.org wrote:
We really should have "test_bit_acquire()" and "set_bit_release()".
I considered using test_bit_acquire() but, as you say, there's no set_bit_release() as yet. I could switch things to initialise the flag to set on key creation and clear the flag instead.
David
On Wed, Mar 19, 2025 at 03:57:46PM +0000, David Howells wrote:
Once a key's reference count has been reduced to 0, the garbage collector thread may destroy it at any time and so key_put() is not allowed to touch the key after that point. The most key_put() is normally allowed to do is to touch key_gc_work as that's a static global variable.
However, in an effort to speed up the reclamation of quota, this is now done in key_put() once the key's usage is reduced to 0 - but now the code is looking at the key after the deadline, which is forbidden.
Fix this by using a flag to indicate that a key can be gc'd now rather than looking at the key's refcount in the garbage collector.
Fixes: 9578e327b2b4 ("keys: update key quotas in key_put()") Reported-by: syzbot+6105ffc1ded71d194d6d@syzkaller.appspotmail.com Signed-off-by: David Howells dhowells@redhat.com Tested-by: syzbot+6105ffc1ded71d194d6d@syzkaller.appspotmail.com cc: Jarkko Sakkinen jarkko@kernel.org cc: Oleg Nesterov oleg@redhat.com cc: Kees Cook kees@kernel.org cc: Hillf Danton hdanton@sina.com, cc: keyrings@vger.kernel.org Cc: stable@vger.kernel.org # v6.10+
include/linux/key.h | 1 + security/keys/gc.c | 4 +++- security/keys/key.c | 2 ++ 3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/key.h b/include/linux/key.h index 074dca3222b9..ba05de8579ec 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -236,6 +236,7 @@ struct key { #define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */ #define KEY_FLAG_KEEP 8 /* set if key should not be removed */ #define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */ +#define KEY_FLAG_FINAL_PUT 10 /* set if final put has happened on key */ /* the key type and key description string * - the desc is used to match a key against search criteria diff --git a/security/keys/gc.c b/security/keys/gc.c index 7d687b0962b1..f27223ea4578 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -218,8 +218,10 @@ static void key_garbage_collector(struct work_struct *work) key = rb_entry(cursor, struct key, serial_node); cursor = rb_next(cursor);
if (refcount_read(&key->usage) == 0)
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */
test_bit() is already atomic.
https://docs.kernel.org/core-api/wrappers/atomic_bitops.html
goto found_unreferenced_key;
}if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) { if (key->type == key_gc_dead_keytype) { diff --git a/security/keys/key.c b/security/keys/key.c index 3d7d185019d3..7198cd2ac3a3 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -658,6 +658,8 @@ void key_put(struct key *key) key->user->qnbytes -= key->quotalen; spin_unlock_irqrestore(&key->user->lock, flags); }
smp_mb(); /* key->user before FINAL_PUT set. */set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
Ditto.
Nit: I'm just thinking should the name imply more like that "now key_put() is actually done". E.g., even something like KEY_FLAG_PUT_DONE would be more self-descriptive.
schedule_work(&key_gc_work); }}
BR, Jarkko
Jarkko Sakkinen jarkko@kernel.org wrote:
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */test_bit() is already atomic.
Atomiticity doesn't apply to test_bit() - it only matters when it does two (or more) accesses that must be perceptually indivisible (e.g. set_bit doing RMW).
But atomiticity isn't the issue here, hence the barrier. You need to be looking at memory-barriers.txt, not atomic_bitops.txt.
We have two things to correctly order and set_bit() does not imply sufficient barriering; test_and_set_bit() does, but not set_bit(), hence Linus's comment about really wanting a set_bit_release().
smp_mb(); /* key->user before FINAL_PUT set. */set_bit(KEY_FLAG_FINAL_PUT, &key->flags);Ditto.
Ditto. ;-)
Nit: I'm just thinking should the name imply more like that "now key_put() is actually done". E.g., even something like KEY_FLAG_PUT_DONE would be more self-descriptive.
KEY_FLAG_PUT_DONE isn't right. There can be lots of puts on a single key - only the one that reduces it to 0 matters for this. You could call it KEY_FLAG_CAN_NOW_GC or KEY_FLAG_GC_ABLE.
David
On Thu, Mar 20, 2025 at 04:39:11PM +0000, David Howells wrote:
Jarkko Sakkinen jarkko@kernel.org wrote:
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */test_bit() is already atomic.
Atomiticity doesn't apply to test_bit() - it only matters when it does two (or more) accesses that must be perceptually indivisible (e.g. set_bit doing RMW).
But atomiticity isn't the issue here, hence the barrier. You need to be looking at memory-barriers.txt, not atomic_bitops.txt.
We have two things to correctly order and set_bit() does not imply sufficient barriering; test_and_set_bit() does, but not set_bit(), hence Linus's comment about really wanting a set_bit_release().
Oops, I was hallucinating here. And yeah, test_and_set_bit() does imply full mb as you said.
I was somehow remembering what I did in SGX driver incorrectly and that led me into misconclusions, sorry.
if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags)) return -EBUSY;
smp_mb(); /* key->user before FINAL_PUT set. */set_bit(KEY_FLAG_FINAL_PUT, &key->flags);Ditto.
Ditto. ;-)
Duh, no need poke with the stick further (or deeper) ;-)
Nit: I'm just thinking should the name imply more like that "now key_put() is actually done". E.g., even something like KEY_FLAG_PUT_DONE would be more self-descriptive.
KEY_FLAG_PUT_DONE isn't right. There can be lots of puts on a single key - only the one that reduces it to 0 matters for this. You could call it KEY_FLAG_CAN_NOW_GC or KEY_FLAG_GC_ABLE.
Well all alternatives are fine but my thinking was that one that finally zeros the refcount, "finalizes put" (pick whatever you want anyway).
David
BR, Jarkko
On Thu, Mar 20, 2025 at 07:28:36PM +0200, Jarkko Sakkinen wrote:
On Thu, Mar 20, 2025 at 04:39:11PM +0000, David Howells wrote:
Jarkko Sakkinen jarkko@kernel.org wrote:
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */test_bit() is already atomic.
Atomiticity doesn't apply to test_bit() - it only matters when it does two (or more) accesses that must be perceptually indivisible (e.g. set_bit doing RMW).
But atomiticity isn't the issue here, hence the barrier. You need to be looking at memory-barriers.txt, not atomic_bitops.txt.
We have two things to correctly order and set_bit() does not imply sufficient barriering; test_and_set_bit() does, but not set_bit(), hence Linus's comment about really wanting a set_bit_release().
Oops, I was hallucinating here. And yeah, test_and_set_bit() does imply full mb as you said.
I was somehow remembering what I did in SGX driver incorrectly and that led me into misconclusions, sorry.
if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags)) return -EBUSY;
smp_mb(); /* key->user before FINAL_PUT set. */set_bit(KEY_FLAG_FINAL_PUT, &key->flags);Ditto.
Ditto. ;-)
Duh, no need poke with the stick further (or deeper) ;-)
Nit: I'm just thinking should the name imply more like that "now key_put() is actually done". E.g., even something like KEY_FLAG_PUT_DONE would be more self-descriptive.
KEY_FLAG_PUT_DONE isn't right. There can be lots of puts on a single key - only the one that reduces it to 0 matters for this. You could call it KEY_FLAG_CAN_NOW_GC or KEY_FLAG_GC_ABLE.
Well all alternatives are fine but my thinking was that one that finally zeros the refcount, "finalizes put" (pick whatever you want anyway).
I'll pick this one up tomorrow and put a PR out within this week.
BR, Jarkko
On Thu, Mar 20, 2025 at 08:46:41PM +0200, Jarkko Sakkinen wrote:
On Thu, Mar 20, 2025 at 07:28:36PM +0200, Jarkko Sakkinen wrote:
On Thu, Mar 20, 2025 at 04:39:11PM +0000, David Howells wrote:
Jarkko Sakkinen jarkko@kernel.org wrote:
if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {smp_mb(); /* Clobber key->user after FINAL_PUT seen. */test_bit() is already atomic.
Atomiticity doesn't apply to test_bit() - it only matters when it does two (or more) accesses that must be perceptually indivisible (e.g. set_bit doing RMW).
But atomiticity isn't the issue here, hence the barrier. You need to be looking at memory-barriers.txt, not atomic_bitops.txt.
We have two things to correctly order and set_bit() does not imply sufficient barriering; test_and_set_bit() does, but not set_bit(), hence Linus's comment about really wanting a set_bit_release().
Oops, I was hallucinating here. And yeah, test_and_set_bit() does imply full mb as you said.
I was somehow remembering what I did in SGX driver incorrectly and that led me into misconclusions, sorry.
if (test_and_set_bit(SGX_ENCL_IOCTL, &encl->flags)) return -EBUSY;
smp_mb(); /* key->user before FINAL_PUT set. */set_bit(KEY_FLAG_FINAL_PUT, &key->flags);Ditto.
Ditto. ;-)
Duh, no need poke with the stick further (or deeper) ;-)
Nit: I'm just thinking should the name imply more like that "now key_put() is actually done". E.g., even something like KEY_FLAG_PUT_DONE would be more self-descriptive.
KEY_FLAG_PUT_DONE isn't right. There can be lots of puts on a single key - only the one that reduces it to 0 matters for this. You could call it KEY_FLAG_CAN_NOW_GC or KEY_FLAG_GC_ABLE.
Well all alternatives are fine but my thinking was that one that finally zeros the refcount, "finalizes put" (pick whatever you want anyway).
I'll pick this one up tomorrow and put a PR out within this week.
I can try get this done tomorrow fully (with only one patch in the PR) so that we would get it still to the ongoing release...
BR, Jarkko
linux-stable-mirror@lists.linaro.org
-
David Howells -
Jarkko Sakkinen -
Linus Torvalds -
Oleg Nesterov