Re: [PATCH net] appletalk: fix use-after-free in atalk_find_primary()
From: Simon Horman <horms@kernel.org>
Date: 2026-06-16 16:34:08
Also in:
lkml, stable
On Mon, Jun 15, 2026 at 06:39:28PM +0800, Yizhou Zhao wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Reported-by: Yuxiang Yang <redacted> Reported-by: Ao Wang <redacted> Reported-by: Xuewei Feng <redacted> Reported-by: Qi Li <redacted> Reported-by: Ke Xu <redacted> Assisted-by: GLM:GLM-5.1 Signed-off-by: Yizhou Zhao <redacted> ---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.
quoted hunk ↗ jump to hunk
-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.
quoted hunk ↗ jump to hunk
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