linux-kernel - Re: [PATCH net v3 1/1] atm: mpoa: Fix UAF on qos

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] [day] [month] [year] [list]

Message-ID: <20251211185002.44e4aee3@kernel.org>
Date: Thu, 11 Dec 2025 18:50:02 +0900
From: Jakub Kicinski <kuba@...nel.org>
To: Minseong Kim <ii4gsp@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
 <horms@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH net v3 1/1] atm: mpoa: Fix UAF on qos_head list in
 procfs

On Thu,  4 Dec 2025 15:24:21 +0900 Minseong Kim wrote:
> Reported-by: Minseong Kim <ii4gsp@...il.com>
> Closes: https://lore.kernel.org/netdev/CAKrymDR1X3XTX_1ZW3XXXnuYH+kzsnv7Av5uivzR1sto+5BFQg@mail.gmail.com/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@...r.kernel.org
> Signed-off-by: Minseong Kim <ii4gsp@...il.com>

The tags are wrong. Reported-by is only used when it's not the same as
author. And you're pointing Closes at previous version? I don't get it.

> -int atm_mpoa_delete_qos(struct atm_mpoa_qos *entry)
> +static int __atm_mpoa_delete_qos_locked(struct atm_mpoa_qos *entry)
>  {
>  	struct atm_mpoa_qos *curr;
>  
> -	if (entry == NULL)
> +	if (!entry)
>  		return 0;
> +

please avoid unrelated code cleanups in fixes

>  	if (entry == qos_head) {
> -		qos_head = qos_head->next;
> -		kfree(entry);
> +		qos_head = entry->next;
>  		return 1;
>  	}
>  
>  	curr = qos_head;
> -	while (curr != NULL) {
> +	while (curr) {
>  		if (curr->next == entry) {
>  			curr->next = entry->next;
> -			kfree(entry);
>  			return 1;
>  		}
>  		curr = curr->next;

> + * Overwrites the old entry or makes a new one.
> + */
> +struct atm_mpoa_qos *atm_mpoa_add_qos(__be32 dst_ip, struct atm_qos *qos)
> +{
> +	struct atm_mpoa_qos *entry;
> +	struct atm_mpoa_qos *new;
> +
> +	/* Fast path: update existing entry */
> +	mutex_lock(&qos_mutex);
> +	entry = __atm_mpoa_search_qos(dst_ip);
> +	if (entry) {
> +		entry->qos = *qos;
> +		mutex_unlock(&qos_mutex);
> +		return entry;
> +	}
> +	mutex_unlock(&qos_mutex);
> +
> +	/* Allocate outside lock */

Why allocate outside the lock? It makes the code more complicated,
keep it simple unless you can prove real life benefits.

> +	new = kmalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return NULL;