Re: [PATCH net v3 1/1] atm: mpoa: Fix UAF on qos_head list in procfs

[Jakub Kicinski]

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

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;