linux-kernel - Re: [PATCH] keys: Fix UAF 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: <20250318112245.GA14792@redhat.com>
Date: Tue, 18 Mar 2025 12:22:46 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: David Howells <dhowells@...hat.com>
Cc: Jarkko Sakkinen <jarkko@...nel.org>, Kees Cook <kees@...nel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Josh Drake <josh@...phoslabs.com>,
	Suraj Sonawane <surajsonawane0215@...il.com>,
	keyrings@...r.kernel.org, linux-security-module@...r.kernel.org,
	security@...nel.org, stable@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] keys: Fix UAF in key_put()

On 03/18, David Howells wrote:
>
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -645,21 +645,30 @@ EXPORT_SYMBOL(key_reject_and_link);
>   */
>  void key_put(struct key *key)
>  {
> +	int quota_flag;
> +	unsigned short len;
> +	struct key_user *user;
> +
>  	if (key) {
>  		key_check(key);
>  
> +		quota_flag = test_bit(KEY_FLAG_IN_QUOTA, &key->flags);
> +		len = key->quotalen;
> +		user = key->user;
> +		refcount_inc(&user->usage);
>  		if (refcount_dec_and_test(&key->usage)) {
>  			unsigned long flags;
>  
>  			/* deal with the user's key tracking and quota */
> -			if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> -				spin_lock_irqsave(&key->user->lock, flags);
> -				key->user->qnkeys--;
> -				key->user->qnbytes -= key->quotalen;
> -				spin_unlock_irqrestore(&key->user->lock, flags);
> +			if (quota_flag) {
> +				spin_lock_irqsave(&user->lock, flags);
> +				user->qnkeys--;
> +				user->qnbytes -= len;
> +				spin_unlock_irqrestore(&user->lock, flags);
>  			}
>  			schedule_work(&key_gc_work);
>  		}
> +		key_user_put(user);

Do we really need the unconditional refcount_inc / key_user_put ?

	void key_put(struct key *key)
	{
		if (key) {
			struct key_user *user = NULL;
			unsigned short len;

			key_check(key);

			if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
				len = key->quotalen;
				user = key->user;
				refcount_inc(&user->usage);
			}

			if (refcount_dec_and_test(&key->usage)) {
				unsigned long flags;

				/* deal with the user's key tracking and quota */
				if (user) {
					spin_lock_irqsave(&user->lock, flags);
					user->qnkeys--;
					user->qnbytes -= len;
					spin_unlock_irqrestore(&user->lock, flags);
				}
				schedule_work(&key_gc_work);
			}

			if (user)
				key_user_put(user);
		}
	}

looks a bit more clear/simple to me...

Oleg.