linux-kernel - Re: [RFC PATCH] keys: flush work when accessing /proc/key-users

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: <67480da4c1418ae03123ecc5cbfb189119d9ca0e.camel@kernel.org>
Date:   Thu, 07 Dec 2023 06:33:19 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Luis Henriques <lhenriques@...e.de>,
        David Howells <dhowells@...hat.com>,
        Eric Biggers <ebiggers@...nel.org>
Cc:     keyrings@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] keys: flush work when accessing /proc/key-users

David, this really needs your feedback.

BR, Jarkko

On Wed, 2023-12-06 at 14:57 +0000, Luis Henriques wrote:
> Make sure the garbage collector has been run before cycling through
> all
> the user keys.
> 
> Signed-off-by: Luis Henriques <lhenriques@...e.de>
> ---
> Hi!
> 
> This patch is mostly for getting some feedback on how to fix an
> fstest
> failing for ext4/fscrypt (generic/581).  Basically, the test relies
> on the
> data read from /proc/key-users to be up-to-date regarding the number
> of
> keys a given user currently has.  However, this file can't be trusted
> because it races against the keys GC.
> 
> Using flush_work() seems to work (I can't reproduce the failure), but
> it
> may be overkill.  Or simply not acceptable.  Maybe, as Eric suggested
> elsewhere [1], there could be a synchronous
> key_put/revoke/invalidate/...,
> which would wait for the key GC to do its work, although that
> probably
> would require some more code re-work.
> 
> [1]
> https://lore.kernel.org/all/20231128173734.GD1148@sol.localdomain/
> 
>  security/keys/gc.c       | 6 ++++++
>  security/keys/internal.h | 1 +
>  security/keys/proc.c     | 1 +
>  3 files changed, 8 insertions(+)
> 
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 3c90807476eb..57b5a54490a0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -44,6 +44,12 @@ struct key_type key_type_dead = {
>  	.name = ".dead",
>  };
>  
> +void key_flush_gc(void)
> +{
> +	kenter("");
> +	flush_work(&key_gc_work);
> +}
> +
>  /*
>   * Schedule a garbage collection run.
>   * - time precision isn't particularly important
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 471cf36dedc0..fee1d0025d96 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -170,6 +170,7 @@ extern void keyring_restriction_gc(struct key
> *keyring,
>  extern void key_schedule_gc(time64_t gc_at);
>  extern void key_schedule_gc_links(void);
>  extern void key_gc_keytype(struct key_type *ktype);
> +extern void key_flush_gc(void);
>  
>  extern int key_task_permission(const key_ref_t key_ref,
>  			       const struct cred *cred,
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index d0cde6685627..2837e00a240a 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -277,6 +277,7 @@ static void *proc_key_users_start(struct seq_file
> *p, loff_t *_pos)
>  	struct rb_node *_p;
>  	loff_t pos = *_pos;
>  
> +	key_flush_gc();
>  	spin_lock(&key_user_lock);
>  
>  	_p = key_user_first(seq_user_ns(p), &key_user_tree);