Re: [PATCH net] appletalk: fix use-after-free in atalk_find_primary()

[Simon Horman <horms@kernel.org>]

On Mon, Jun 15, 2026 at 06:39:28PM +0800, Yizhou Zhao wrote:
> atalk_find_primary() walks the global AppleTalk interface list under
> atalk_interfaces_lock, but returns a pointer to iface->address after
> dropping that lock.  Both atalk_autobind() and atalk_bind() then
> dereference the returned pointer without any lifetime protection.
> 
> The interface can be removed concurrently through the normal AppleTalk
> interface ioctl path.  SIOCATALKDIFADDR calls atalk_dev_down(), which
> eventually reaches atif_drop_device() and frees the same struct
> atalk_iface that owns the returned address field.  A racing bind can
> therefore read from freed memory.
> 
> This is reachable with a configured AppleTalk interface; reproducing the
> race does not require a malicious device or driver.  The configuration
> ioctls require CAP_NET_ADMIN in the initial user namespace, and
> AF_APPLETALK sockets are limited to init_net.
> 
> Fix the lifetime issue without changing the returned address pointer
> type.  Rename the helper to atalk_find_primary_locked() and keep
> atalk_interfaces_lock held across the return.  The callers now copy
> s_net and s_node while the lock is still held, then immediately release
> the lock before doing any further work.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
> Reported-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
> Reported-by: Ao Wang <wangao@seu.edu.cn>
> Reported-by: Xuewei Feng <fengxw06@126.com>
> Reported-by: Qi Li <qli01@tsinghua.edu.cn>
> Reported-by: Ke Xu <xuke@tsinghua.edu.cn>
> Assisted-by: GLM:GLM-5.1
> Signed-off-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
> ---
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 30a6dc06291c..4d6576cd0ae8 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -351,7 +351,7 @@ struct atalk_addr *atalk_find_dev_addr(struct net_device *dev)
>  	return iface ? &iface->address : NULL;
>  }
>  

A kernel doc for atalk_find_dev_addr which describes the locking
expectations is probably warranted here.

> -static struct atalk_addr *atalk_find_primary(void)
> +static struct atalk_addr *atalk_find_primary_locked(void)
>  {
>  	struct atalk_iface *fiface = NULL;
>  	struct atalk_addr *retval;
> @@ -378,7 +378,6 @@ static struct atalk_addr *atalk_find_primary(void)
>  	else
>  		retval = NULL;
>  out:
> -	read_unlock_bh(&atalk_interfaces_lock);

This function still acquires atalk_interfaces_lock but I don't think that
asymmetry is justified. If the critical section needs to be expanded then I
think it would be best to both acquire and release the lock in the caller.

>  	return retval;
>  }
>  
> @@ -1132,20 +1131,24 @@ static int atalk_autobind(struct sock *sk)
>  {
>  	struct atalk_sock *at = at_sk(sk);
>  	struct sockaddr_at sat;
> -	struct atalk_addr *ap = atalk_find_primary();
> +	struct atalk_addr *ap = atalk_find_primary_locked();
>  	int n = -EADDRNOTAVAIL;

We could take this opportunity to move towards reverse xmas tree here.

>  
>  	if (!ap || ap->s_net == htons(ATADDR_ANYNET))
> -		goto out;
> +		goto unlock_and_out;
>  
>  	at->src_net  = sat.sat_addr.s_net  = ap->s_net;
>  	at->src_node = sat.sat_addr.s_node = ap->s_node;
> +	read_unlock_bh(&atalk_interfaces_lock);

The unlock_and_out label applies to the critical section which ends here.
But in my mind the goto construct is best used for handling errors 
that apply to, and in general accumulate during, the flow of a function.

Combining that with my earlier comments would go for something like the
following (completely untested!). Similarly in atalk_bind().

	struct atalk_sock *at = at_sk(sk);
	struct sockaddr_at sat;
	int n = -EADDRNOTAVAIL;
	struct atalk_addr *ap;

	read_lock_bh(&atalk_interfaces_lock);
	ap = atalk_find_primary_locked();

	if (ap && ap->s_net != htons(ATADDR_ANYNET)) {
		at->src_net  = sat.sat_addr.s_net  = ap->s_net;
		at->src_node = sat.sat_addr.s_node = ap->s_node;
	}

	read_unlock_bh(&atalk_interfaces_lock);

>  
>  	n = atalk_pick_and_bind_port(sk, &sat);
>  	if (!n)
>  		sock_reset_flag(sk, SOCK_ZAPPED);
>  out:
>  	return n;
> +unlock_and_out:
> +	read_unlock_bh(&atalk_interfaces_lock);
> +	goto out;
>  }
>  
>  /* Set the address 'our end' of the connection */

...

-- 
pw-bot: changes-requested