linux-kernel - Re: [RFC PATCH v2] keys: update key quotas in key

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bk988450.fsf@suse.de>
Date: Fri, 26 Jan 2024 16:12:59 +0000
From: Luis Henriques <lhenriques@...e.de>
To: Eric Biggers <ebiggers@...nel.org>
Cc: David Howells <dhowells@...hat.com>,  Jarkko Sakkinen
 <jarkko@...nel.org>,  keyrings@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] keys: update key quotas in key_put()

Eric Biggers <ebiggers@...nel.org> writes:

> On Mon, Jan 15, 2024 at 12:03:00PM +0000, Luis Henriques wrote:
>> Delaying key quotas update when key's refcount reaches 0 in key_put() has
>> been causing some issues in fscrypt testing.  This patches fixes this test
>> flakiness by dealing with the quotas immediately, but leaving all the other
>> clean-ups to the key garbage collector.  Unfortunately, this means that we
>> also need to switch to the irq-version of the spinlock that protects quota.
>> 
>> Signed-off-by: Luis Henriques <lhenriques@...e.de>
>> ---
>> Hi David!
>> 
>> I have these changes in my local disk for a while; I wanted to send them
>> before EOY break but... yeah, it didn't happen.  Anyway, I'm still sending
>> it as an RFC as I'm probably missing something.
>> 
>>  security/keys/gc.c     |  8 --------
>>  security/keys/key.c    | 32 ++++++++++++++++++++++----------
>>  security/keys/keyctl.c | 11 ++++++-----
>>  3 files changed, 28 insertions(+), 23 deletions(-)
>
> This patch seems reasonable to me, though I'm still thinking about changing
> fs/crypto/ to manage its key quotas itself which would avoid the issue entirely.
>
> Note that as I said before, fs/crypto/ does key_put() on a whole keyring at
> once, in order to release the quota of the keys in the keyring.  Do you plan to
> also change fs/crypto/ to keyring_clear() the keyring before putting it?
> Without that, I don't think the problem is solved, as the quota release will
> still happen asynchronously due to the keyring being cleared asynchronously.

Ah, good point.  In the meantime I had forgotten everything about this
code and missed that.  So, I can send another patch to fs/crypto to add
that extra call once (or if) this patch is accepted.

If I'm reading the code correctly, the only place where this extra call is
required is on fscrypt_put_master_key():

diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 0edf0b58daa7..4afd32f1aed9 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -74,6 +74,7 @@ void fscrypt_put_master_key(struct fscrypt_master_key *mk)
 	 * that concurrent keyring lookups can no longer find it.
 	 */
 	WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 0);
+	keyring_clear(mk->mk_users);
 	key_put(mk->mk_users);
 	mk->mk_users = NULL;
 	call_rcu(&mk->mk_rcu_head, fscrypt_free_master_key);

On the other hand, if you're really working towards dropping this code
entirely, maybe there's not point doing that.

Cheers,
-- 
Luís