linux-kernel - Re: [PATCH] KEYS: Fix RCU handling in key_gc

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]

Date:	Tue, 4 May 2010 17:59:53 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	David Howells <dhowells@...hat.com>
Cc:	torvalds@...l.org, akpm@...ux-foundation.org, serue@...ibm.com,
	keyrings@...ux-nfs.org, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KEYS: Fix RCU handling in key_gc_keyring()

On Tue, May 04, 2010 at 02:16:10PM +0100, David Howells wrote:
> key_gc_keyring() needs to either hold the RCU read lock or hold the keyring
> semaphore if it's going to scan the keyring's list.  Given that it only needs
> to read the key list, and it's doing so under a spinlock, the RCU read lock is
> the thing to use.
> 
> Furthermore, the RCU check added in e7b0a61b7929632d36cf052d9e2820ef0a9c1bfe is
> incorrect as holding the spinlock on key_serial_lock is not grounds for
> assuming a keyring's pointer list can be read safely.  Instead, a simple
> rcu_dereference() inside of the previously mentioned RCU read lock is what we
> want.

As the guilty party in e7b0a61b7929632d36cf052d9e2820ef0a9c1bfe...  ;-)

Acked-by: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>

> Reported-by: Serge E. Hallyn <serue@...ibm.com>
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
> 
>  security/keys/gc.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 1990231..a46e825 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -77,10 +77,10 @@ static bool key_gc_keyring(struct key *keyring, time_t limit)
>  		goto dont_gc;
> 
>  	/* scan the keyring looking for dead keys */
> -	klist = rcu_dereference_check(keyring->payload.subscriptions,
> -				      lockdep_is_held(&key_serial_lock));
> +	rcu_read_lock();
> +	klist = rcu_dereference(keyring->payload.subscriptions);
>  	if (!klist)
> -		goto dont_gc;
> +		goto unlock_dont_gc;
> 
>  	for (loop = klist->nkeys - 1; loop >= 0; loop--) {
>  		key = klist->keys[loop];
> @@ -89,11 +89,14 @@ static bool key_gc_keyring(struct key *keyring, time_t limit)
>  			goto do_gc;
>  	}
> 
> +unlock_dont_gc:
> +	rcu_read_unlock();
>  dont_gc:
>  	kleave(" = false");
>  	return false;
> 
>  do_gc:
> +	rcu_read_unlock();
>  	key_gc_cursor = keyring->serial;
>  	key_get(keyring);
>  	spin_unlock(&key_serial_lock);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/