[PATCH 6.18 024/109] ax25: wait for ax25_send_frame() before detaching the connections of a device

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

6.18-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Bernard Pidoux <bernard.f6bvp@gmail.com>

A connection created by ax25_send_frame(), for ROSE, NET/ROM or IP
over AX.25, points to the ax25_dev of its device without holding a
reference on it. It relies on ax25_kill_by_device() to detach it when
the device goes down, before ax25_dev_device_down() frees the ax25_dev.

On NETDEV_DOWN the two run one after the other. In between, the
ax25_dev is still on ax25_dev_list, so ax25_send_frame() still finds
it, creates a connection and adds it to ax25_list after
ax25_kill_by_device() has finished walking that list. The connection is
left with a pointer to an ax25_dev that is freed a moment later, which
ax25_find_cb() and the AX.25 timers then dereference:

  BUG: KASAN: slab-use-after-free in ax25_find_cb+0x2ed/0x330 [ax25]
  Read of size 8 at addr ffff8880031e8c10 by task repro/3287
   ax25_find_cb+0x2ed/0x330 [ax25]
   rose_transmit_link+0xea/0x560 [rose]
   rose_write_internal+0x3ab/0x15b0 [rose]
   rose_release+0x2b2/0x4b0 [rose]
  Allocated by: ax25_dev_device_up, ax25_device_event
  Freed by: kfree_rcu_monitor, queued from ax25_device_event
  The buggy address is located 16 bytes inside of
   freed 256-byte region, cache kmalloc-256

This is the trace of the syzbot report below. An earlier commit, "rose:
use timer_shutdown_sync() for t0timer teardown", attributed that
report to the ROSE t0 timer; the freed object in the report is the
ax25_dev, as it is here.

A connection that is on the list in time is not safe either while it
is still being set up: ax25_kill_by_device() can detach it, clearing
its ax25_dev pointer, between the moment ax25_send_frame() adds it and
the moment it starts the link and queues the frame, which then
dereference NULL.

Make the two exclude each other with RCU. ax25_send_frame() now runs
entirely under rcu_read_lock() and gives up when the device is no
longer marked up. ax25_kill_by_device() clears device_up and calls
synchronize_rcu() before it walks the list: once that returns, every
connection ax25_send_frame() was working on is complete and on the
list, and no new one can appear behind the walk. This adds one RCU
grace period to taking an AX.25 interface down.

Tested on 6.12.94 with KASAN and lockdep, with the modules of this
tree and the rest of this series:

 - with a 100 ms delay inserted between ax25_kill_by_device() and
   ax25_dev_device_down(), a single "ifconfig bpq0 down" during eight
   ROSE call requests left connections on the list and ended in a
   use-after-free in ax25_heartbeat_expiry(); with the series the list
   is empty after the down and there is no report;

 - the syzbot C reproducer, with the interface brought up again in a
   loop, caused 217 KASAN reports and a panic after 41 seconds; with
   the series it ran for 600 seconds without any report;

 - 60 plain "ifconfig bpq0 down" / "up" under 20 ROSE call requests
   per second, and a ROSE call through a three node chain over mkiss,
   behave as before;

 - eight ROSE calls started at the same instant still end up on a
   single AX.25 connection.

Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
Assisted-by: Claude:claude-opus-5-5
(cherry picked from commit 45071a40f7d5db10350dbd177decd3f68fdaaa01)
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 net/ax25/af_ax25.c  | 12 +++++++++++-
 net/ax25/ax25_out.c | 38 ++++++++++++++++++++------------------
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 76f6848fba7d6..81c9631e4c41e 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -81,7 +81,17 @@ static void ax25_kill_by_device(struct net_device *dev)
 
 	if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
 		return;
-	ax25_dev->device_up = false;
+	WRITE_ONCE(ax25_dev->device_up, false);
+
+	/*
+	 * ax25_send_frame() creates connections that hold no reference on
+	 * the ax25_dev: they rely on the walk below to detach them before the
+	 * ax25_dev is freed. It runs under rcu_read_lock() and gives up when
+	 * device_up is clear, so once this returns every connection it was
+	 * setting up is complete and on the list, and no new one can appear
+	 * behind the walk.
+	 */
+	synchronize_rcu();
 
 	spin_lock_bh(&ax25_list_lock);
 again:
diff --git a/net/ax25/ax25_out.c b/net/ax25/ax25_out.c
index eeb2aa1e2186f..f082b69126623 100644
--- a/net/ax25/ax25_out.c
+++ b/net/ax25/ax25_out.c
@@ -34,34 +34,34 @@ ax25_cb *ax25_send_frame(struct sk_buff *skb, int paclen, const ax25_address *sr
 	ax25_dev *ax25_dev;
 	ax25_cb *ax25, *existing;
 
+	/*
+	 * The connection made or used here holds no reference on the
+	 * ax25_dev. ax25_kill_by_device() clears device_up, waits for the RCU
+	 * readers and only then detaches the connections of the device: stay
+	 * under the RCU read lock until the frame is queued, and do not start
+	 * anything on a device that is going down.
+	 */
+	rcu_read_lock();
+	ax25_dev = ax25_dev_ax25dev(dev);
+	if (!ax25_dev || !READ_ONCE(ax25_dev->device_up)) {
+		rcu_read_unlock();
+		return NULL;
+	}
+
 	/*
 	 * Take the default packet length for the device if zero is
 	 * specified.
 	 */
-	if (paclen == 0) {
-		rcu_read_lock();
-		ax25_dev = ax25_dev_ax25dev(dev);
-		if (!ax25_dev) {
-			rcu_read_unlock();
-			return NULL;
-		}
+	if (paclen == 0)
 		paclen = ax25_dev->values[AX25_VALUES_PACLEN];
-		rcu_read_unlock();
-	}
 
 	/*
 	 * Look for an existing connection.
 	 */
 	if ((ax25 = ax25_find_cb(src, dest, digi, dev)) != NULL) {
 		ax25_output(ax25, paclen, skb);
-		return ax25;		/* It already existed */
-	}
-
-	rcu_read_lock();
-	ax25_dev = ax25_dev_ax25dev(dev);
-	if (!ax25_dev) {
 		rcu_read_unlock();
-		return NULL;
+		return ax25;		/* It already existed */
 	}
 
 	if ((ax25 = ax25_create_cb()) == NULL) {
@@ -69,7 +69,6 @@ ax25_cb *ax25_send_frame(struct sk_buff *skb, int paclen, const ax25_address *sr
 		return NULL;
 	}
 	ax25_fillin_cb(ax25, ax25_dev);
-	rcu_read_unlock();
 
 	ax25->source_addr = *src;
 	ax25->dest_addr   = *dest;
@@ -78,6 +77,7 @@ ax25_cb *ax25_send_frame(struct sk_buff *skb, int paclen, const ax25_address *sr
 		ax25->digipeat = kmemdup(digi, sizeof(*digi), GFP_ATOMIC);
 		if (ax25->digipeat == NULL) {
 			ax25_cb_put(ax25);
+			rcu_read_unlock();
 			return NULL;
 		}
 	}
@@ -91,10 +91,11 @@ ax25_cb *ax25_send_frame(struct sk_buff *skb, int paclen, const ax25_address *sr
 	if (existing) {
 		ax25_cb_put(ax25);
 		ax25_output(existing, paclen, skb);
+		rcu_read_unlock();
 		return existing;
 	}
 
-	switch (ax25->ax25_dev->values[AX25_VALUES_PROTOCOL]) {
+	switch (ax25_dev->values[AX25_VALUES_PROTOCOL]) {
 	case AX25_PROTO_STD_SIMPLEX:
 	case AX25_PROTO_STD_DUPLEX:
 		ax25_std_establish_data_link(ax25);
@@ -121,6 +122,7 @@ ax25_cb *ax25_send_frame(struct sk_buff *skb, int paclen, const ax25_address *sr
 	ax25_start_heartbeat(ax25);
 
 	ax25_output(ax25, paclen, skb);
+	rcu_read_unlock();
 
 	return ax25;			/* We had to create it */
 }
-- 
2.53.0