[PATCH net] appletalk: fix use-after-free in atalk_sendmsg due to route lookup race

[PATCH net] appletalk: fix use-after-free in atalk_sendmsg due to route lookup race

[Zhenghang Xiao]

atrtr_find() returns a raw pointer after releasing atalk_routes_lock.
In atalk_sendmsg(), both the primary route (rt) and loopback route
(rt_lo) pointers are held across sock_alloc_send_skb() which can sleep.
Concurrent atrtr_delete() or atrtr_device_down() can kfree() the route
during this window, causing UAF when rt->flags, rt->gateway, or
rt_lo->dev are accessed.

Fix by deferring route frees with kfree_rcu() and caching all needed
route fields under rcu_read_lock() before the sleeping allocation.
Take dev_hold() on both route devices to prevent disappearance after
the route's dev_put() in the delete path.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zhenghang Xiao <kipreyyy@gmail.com>
---
 include/linux/atalk.h |  1 +
 net/appletalk/ddp.c   | 52 +++++++++++++++++++++++++++----------------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/include/linux/atalk.h b/include/linux/atalk.h
index a55bfc6567d0..932cb72379c2 100644
--- a/include/linux/atalk.h
+++ b/include/linux/atalk.h
@@ -12,6 +12,7 @@ struct atalk_route {
 	struct atalk_addr  gateway;
 	int		   flags;
 	struct atalk_route *next;
+	struct rcu_head	   rcu;
 };
 
 /**
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 30a6dc06291c..2c754378fa08 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -603,7 +603,7 @@ static int atrtr_delete(struct atalk_addr *addr)
 		     tmp->target.s_node == addr->s_node)) {
 			*r = tmp->next;
 			dev_put(tmp->dev);
-			kfree(tmp);
+			kfree_rcu(tmp, rcu);
 			goto out;
 		}
 		r = &tmp->next;
@@ -628,7 +628,7 @@ static void atrtr_device_down(struct net_device *dev)
 		if (tmp->dev == dev) {
 			*r = tmp->next;
 			dev_put(dev);
-			kfree(tmp);
+			kfree_rcu(tmp, rcu);
 		} else
 			r = &tmp->next;
 	}
@@ -1553,10 +1553,13 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	int loopback = 0;
 	struct sockaddr_at local_satalk, gsat;
 	struct sk_buff *skb;
-	struct net_device *dev;
+	struct net_device *dev = NULL;
+	struct net_device *dev_lo = NULL;
 	struct ddpehdr *ddp;
 	int size, hard_header_len;
 	struct atalk_route *rt, *rt_lo = NULL;
+	int rt_flags;
+	struct atalk_addr rt_gateway;
 	int err;
 
 	if (flags & ~(MSG_DONTWAIT|MSG_CMSG_COMPAT))
@@ -1600,6 +1603,7 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	/* For headers */
 	size = sizeof(struct ddpehdr) + len + ddp_dl->header_length;
 
+	rcu_read_lock();
 	if (usat->sat_addr.s_net || usat->sat_addr.s_node == ATADDR_ANYNODE) {
 		rt = atrtr_find(&usat->sat_addr);
 	} else {
@@ -1610,29 +1614,38 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 
 		rt = atrtr_find(&at_hint);
 	}
-	err = -ENETUNREACH;
-	if (!rt)
+	if (!rt) {
+		rcu_read_unlock();
+		err = -ENETUNREACH;
 		goto out;
+	}
 
 	dev = rt->dev;
-
-	net_dbg_ratelimited("SK %p: Size needed %d, device %s\n",
-			sk, size, dev->name);
+	dev_hold(dev);
+	rt_flags = rt->flags;
+	rt_gateway = rt->gateway;
 
 	hard_header_len = dev->hard_header_len;
 	/* Leave room for loopback hardware header if necessary */
 	if (usat->sat_addr.s_node == ATADDR_BCAST &&
-	    (dev->flags & IFF_LOOPBACK || !(rt->flags & RTF_GATEWAY))) {
+	    (dev->flags & IFF_LOOPBACK || !(rt_flags & RTF_GATEWAY))) {
 		struct atalk_addr at_lo;
 
 		at_lo.s_node = 0;
 		at_lo.s_net  = 0;
 
 		rt_lo = atrtr_find(&at_lo);
-
-		if (rt_lo && rt_lo->dev->hard_header_len > hard_header_len)
-			hard_header_len = rt_lo->dev->hard_header_len;
+		if (rt_lo) {
+			dev_lo = rt_lo->dev;
+			dev_hold(dev_lo);
+			if (dev_lo->hard_header_len > hard_header_len)
+				hard_header_len = dev_lo->hard_header_len;
+		}
 	}
+	rcu_read_unlock();
+
+	net_dbg_ratelimited("SK %p: Size needed %d, device %s\n",
+			sk, size, dev->name);
 
 	size += hard_header_len;
 	release_sock(sk);
@@ -1675,7 +1688,7 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	 * to group we are in)
 	 */
 	if (ddp->deh_dnode == ATADDR_BCAST &&
-	    !(rt->flags & RTF_GATEWAY) && !(dev->flags & IFF_LOOPBACK)) {
+	    !(rt_flags & RTF_GATEWAY) && !(dev->flags & IFF_LOOPBACK)) {
 		struct sk_buff *skb2 = skb_copy(skb, GFP_KERNEL);
 
 		if (skb2) {
@@ -1693,19 +1706,18 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		/* loop back */
 		skb_orphan(skb);
 		if (ddp->deh_dnode == ATADDR_BCAST) {
-			if (!rt_lo) {
+			if (!dev_lo) {
 				kfree_skb(skb);
 				err = -ENETUNREACH;
 				goto out;
 			}
-			dev = rt_lo->dev;
-			skb->dev = dev;
+			skb->dev = dev_lo;
 		}
-		ddp_dl->request(ddp_dl, skb, dev->dev_addr);
+		ddp_dl->request(ddp_dl, skb, skb->dev->dev_addr);
 	} else {
 		net_dbg_ratelimited("SK %p: send out.\n", sk);
-		if (rt->flags & RTF_GATEWAY) {
-		    gsat.sat_addr = rt->gateway;
+		if (rt_flags & RTF_GATEWAY) {
+		    gsat.sat_addr = rt_gateway;
 		    usat = &gsat;
 		}
 
@@ -1718,6 +1730,8 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 
 out:
 	release_sock(sk);
+	dev_put(dev);
+	dev_put(dev_lo);
 	return err ? : len;
 }
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH net] appletalk: fix use-after-free in atalk_sendmsg due to route lookup race

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: appletalk: fix use-after-free in atalk_sendmsg due to route lookup race

This patch attempts to fix a use-after-free in atalk_sendmsg() by deferring 
route deletion with kfree_rcu() and caching required route fields under 
rcu_read_lock(). It also takes a reference on the route devices using 
dev_hold() to prevent them from disappearing during the sleeping allocation.

> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 30a6dc06291c3..2c754378fa088 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -603,7 +603,7 @@ static int atrtr_delete(struct atalk_addr *addr)
>  		     tmp->target.s_node == addr->s_node)) {
>  			*r = tmp->next;
>  			dev_put(tmp->dev);
> -			kfree(tmp);
> +			kfree_rcu(tmp, rcu);

Simon says: while the issue below is flagged as not being introduced by
            this patch, I think that if it is problem then it prevents
	    this patch from working as intended.

	    So if it is an issue I suggest addressing it in the context of
	    this patch, perhaps by adding another patch to this patch-set.

[Severity: High]
This isn't a bug introduced by this patch, but does the synchronous
dev_put() allow the network device reference count to drop to zero while
RCU readers are active?

While kfree_rcu() defers freeing the route, dev_put(tmp->dev) is still
called synchronously here. If a device is being unregistered and this
synchronous dev_put() removes the last reference, the device's refcount
could drop to zero before the RCU grace period ends.

When a concurrent atalk_sendmsg() holding rcu_read_lock() then calls
dev_hold() on this device, could it increment a zero refcount and cause a
refcount saturation warning or use-after-free?

[ ... ]

> @@ -628,7 +628,7 @@ static void atrtr_device_down(struct net_device *dev)
>  		if (tmp->dev == dev) {
>  			*r = tmp->next;
>  			dev_put(dev);
> -			kfree(tmp);
> +			kfree_rcu(tmp, rcu);

Simon says: likewise for the issue below.

[Severity: High]
This is a pre-existing issue, but this appears to have the same synchronous
dev_put() problem as atrtr_delete() above.

[ ... ]

> @@ -1600,6 +1603,7 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  	/* For headers */
>  	size = sizeof(struct ddpehdr) + len + ddp_dl->header_length;
>  
> +	rcu_read_lock();
>  	if (usat->sat_addr.s_net || usat->sat_addr.s_node == ATADDR_ANYNODE) {
>  		rt = atrtr_find(&usat->sat_addr);

Simon says: and the issue below too.

[Severity: High]
This isn't a bug introduced by this patch, but are there other callers
of atrtr_find() that are now missing rcu_read_lock() protection?

atrtr_get_dev() calls atrtr_find(), which temporarily takes
atalk_routes_lock and then returns a raw pointer:

net/appletalk/ddp.c:atrtr_get_dev() {
	struct atalk_route *atr = atrtr_find(sa);
	return atr ? atr->dev : NULL;
}

Since atrtr_get_dev() does not hold rcu_read_lock() when dereferencing
atr->dev, and this patch uses kfree_rcu() to defer route deletion, could
a concurrent route deletion free the route before the read occurs?

>  	} else {
> @@ -1610,29 +1614,38 @@ static int atalk_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  
>  		rt = atrtr_find(&at_hint);
>  	}
> -	err = -ENETUNREACH;
> -	if (!rt)
> +	if (!rt) {
> +		rcu_read_unlock();
> +		err = -ENETUNREACH;
>  		goto out;
> +	}
>  
>  	dev = rt->dev;
> -
> -	net_dbg_ratelimited("SK %p: Size needed %d, device %s\n",
> -			sk, size, dev->name);
> +	dev_hold(dev);

Simon says: on the other hand, this one looks like it is orthogonal to
            this patch and should be considered in the context of possible
	    follow-up.

[Severity: Critical]
This is a pre-existing issue, but is there a data race on atrtr_default.dev
that causes a NULL pointer dereference here?

atrtr_device_down() sets the static atrtr_default.dev locklessly to NULL:

net/appletalk/ddp.c:atrtr_device_down() {
...
	if (atrtr_default.dev == dev)
		atrtr_set_default(NULL);
}

If atrtr_find() locklessly returns &atrtr_default, and atrtr_device_down()
clears the default route between the lookup and dev = rt->dev, could this
read a NULL pointer?

Since dev_hold(dev) does not check for NULL, wouldn't it attempt to
increment the refcount at a NULL offset and cause a kernel crash?

Simon says: While this one seems to warrant further investigation in
            the context of this patch.

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of reference
counting on the default route break the newly added RCU assumptions?

The static atrtr_default route does not hold a reference count on its
network device (no dev_hold() is performed when atrtr_set_default()
sets it). Consequently, when the device is unregistered, its refcount
can drop to zero immediately.

If atalk_sendmsg() retrieves this route and calls dev_hold() inside its
RCU critical section, could it increment a zero refcount? This would trigger
the same refcount saturation and use-after-free issues as the synchronous
dev_put() described earlier.

> +	rt_flags = rt->flags;
> +	rt_gateway = rt->gateway;
>  
>  	hard_header_len = dev->hard_header_len;