[PATCH] xen-netback: wait for RCU readers before freeing queues on error

[PATCH] xen-netback: wait for RCU readers before freeing queues on error

[Yehyeong Lee]

The connect() error path frees vif->queues without first waiting for
RCU readers, unlike backend_disconnect() which publishes num_queues = 0,
calls synchronize_net() and only then frees the array.

The net device is registered in xenvif_alloc() during probe, so it is
already live when connect() runs. xenvif_get_stats64() and
xenvif_get_ethtool_stats() read it through the usual stats paths, taking
rcu_read_lock(), sampling num_queues with READ_ONCE() and then
dereferencing vif->queues[i].

A guest can drive connect() into the error path by handing the backend a
bad ring reference or event channel for one of its queues, which makes
connect_data_rings() fail and connect() jump to err: with num_queues
still counting the queues already set up. The error path calls
vfree(vif->queues) without a preceding synchronize_net(), so a reader
that has already sampled a non-zero num_queues and loaded the vif->queues
pointer goes on to dereference the freed vmalloc array.

KASAN catches the read of the freed queue array in xenvif_get_stats()
while a guest repeatedly forces the error path and dom0 reads
/proc/net/dev. Generic KASAN labels a freed vmalloc region
vmalloc-out-of-bounds, since the use-after-free label is reserved for
slab and page objects:

  [  160.524029] ==================================================================
  [  160.527557] BUG: KASAN: vmalloc-out-of-bounds in xenvif_get_stats+0x28f/0x390
  [  160.530848] Read of size 8 at addr ffffc90005de06a8 by task cat/10519
  [  160.533831]
  [  160.534844] CPU: 1 UID: 0 PID: 10519 Comm: cat Not tainted 7.3.0-kCTLwide2-15482-g21a7f44fd076-dirty #2 PREEMPT(full)
  [  160.534849] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  [  160.534855] Call Trace:
  [  160.534868]  <TASK>
  [  160.534870]  dump_stack_lvl+0x64/0x80
  [  160.534885]  print_report+0xd0/0x630
  [  160.534918]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
  [  160.534933]  ? xenvif_get_stats+0x28f/0x390
  [  160.534938]  kasan_report+0xce/0x100
  [  160.534943]  ? xenvif_get_stats+0x28f/0x390
  [  160.534947]  xenvif_get_stats+0x28f/0x390
  [  160.534952]  dev_get_stats+0x2db/0xb50
  [  160.534961]  ? dev_seq_printf_stats+0x446/0x7b0
  [  160.534979]  dev_seq_printf_stats+0x9e/0x7b0
  [  160.534982]  ? __pfx_dev_seq_printf_stats+0x10/0x10
  [  160.534987]  ? dev_seq_next+0xe0/0x1b0
  [  160.534990]  ? __pfx_dev_seq_next+0x10/0x10
  [  160.534993]  ? __asan_memcpy+0x3c/0x60
  [  160.534997]  dev_seq_show+0xf/0x30
  [  160.535000]  seq_read_iter+0x8f3/0x1190
  [  160.535018]  ? alloc_pipe_info+0x2c2/0x430
  [  160.535030]  seq_read+0x267/0x3d0
  [  160.535033]  ? __pfx_seq_read+0x10/0x10
  [  160.535038]  ? __pfx_do_mmap+0x10/0x10
  [  160.535054]  proc_reg_read+0x1ab/0x270
  [  160.535077]  vfs_read+0x177/0xa20
  [  160.535085]  ? __pfx_vfs_read+0x10/0x10
  [  160.535089]  ? vm_mmap_pgoff+0x24b/0x310
  [  160.535108]  ? __pfx_vm_mmap_pgoff+0x10/0x10
  [  160.535116]  ? fdget_pos+0x1ca/0x4b0
  [  160.535124]  ksys_read+0xf7/0x1c0
  [  160.535128]  ? __pfx_ksys_read+0x10/0x10
  [  160.535132]  do_syscall_64+0xe2/0x4b0
  [  160.535141]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  [  160.535149] RIP: 0033:0x463aa1
  [  160.535156] Code: 00 4c 89 d2 4c 89 c6 e9 fd fd ff ff 0f 1f 44 00 00 31 c0 c3 0f 1f 44 00 00 f3 0f 1e fa 80 3d ec 22 1a 00 00 74 13 31 c0 0f 05 <48> 3d 00 f0 ff ff 77 4f c3 66 0f 1f 44 00 00 55 48 89 e5 48 83 ec
  [  160.535168] RSP: 002b:00007fff303dbeb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
  [  160.535176] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000463aa1
  [  160.535178] RDX: 0000000000010000 RSI: 00007f9becf9f000 RDI: 0000000000000003
  [  160.535180] RBP: 00007fff303dbee0 R08: ffffffffffffffff R09: 0000000000000000
  [  160.535182] R10: 0000000000000022 R11: 0000000000000246 R12: 00007f9becf9f000
  [  160.535183] R13: 0000000000010000 R14: 0000000000000001 R15: 0000000000000000
  [  160.535186]  </TASK>
  [  160.535188]
  [  160.636633] The buggy address belongs to a vmalloc virtual mapping
  [  160.639416] Memory state around the buggy address:
  [  160.641738]  ffffc90005de0580: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  [  160.662534]  ffffc90005de0600: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  [  160.665865] >ffffc90005de0680: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  [  160.669222]                                   ^
  [  160.671430]  ffffc90005de0700: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  [  160.674771]  ffffc90005de0780: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  [  160.678102] ==================================================================

Mirror the teardown that backend_disconnect() already performs. Snapshot
num_queues, publish num_queues = 0, call synchronize_net() to wait for
in-flight readers, and only then deinit and free the array using the
snapshot.

Fixes: b17075d5c198 ("xen-netback: fix race condition on XenBus disconnect")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/net/xen-netback/xenbus.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index e020f44ac2c717..d1c9e38cfb8ef2 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -835,10 +835,21 @@ static void connect(struct backend_info *be)
 	return;
 
 err:
-	if (be->vif->num_queues > 0)
+	if (be->vif->num_queues > 0) {
+		unsigned int num_queues = be->vif->num_queues;
+
 		xenvif_disconnect_data(be->vif); /* Clean up existing queues */
-	for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
-		xenvif_deinit_queue(&be->vif->queues[queue_index]);
+
+		/*
+		 * Mirror backend_disconnect(): publish num_queues == 0 and
+		 * synchronize_net() so RCU stats readers finish before the free.
+		 */
+		be->vif->num_queues = 0;
+		synchronize_net();
+
+		for (queue_index = 0; queue_index < num_queues; ++queue_index)
+			xenvif_deinit_queue(&be->vif->queues[queue_index]);
+	}
 	vfree(be->vif->queues);
 	be->vif->queues = NULL;
 	be->vif->num_queues = 0;
-- 
2.43.0

Re: [PATCH] xen-netback: wait for RCU readers before freeing queues on error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves the use-after-free open when the very first queue
  fails, which is every failure for a single-queue frontend.
- [Low] The commit message names xenvif_get_stats64() as one of the RCU
  readers, but xen-netback has no such function.

--

xen-netback: wait for RCU readers before freeing queues on error

This patch changes the connect() error path in xen-netback. It now
publishes num_queues = 0 and calls synchronize_net() before it deinits
and frees vif->queues, which is what backend_disconnect() already does.
The goal is to stop RCU stats readers from dereferencing the freed
vmalloc array.

> The net device is registered in xenvif_alloc() during probe, so it is
> already live when connect() runs. xenvif_get_stats64() and
> xenvif_get_ethtool_stats() read it through the usual stats paths, taking

[Severity: Low]
This isn't a bug, but xen-netback has no xenvif_get_stats64(). The driver
only implements the legacy callback:

drivers/net/xen-netback/interface.c:
	.ndo_get_stats	= xenvif_get_stats,

The KASAN trace also shows that function (xenvif_get_stats+0x28f). Should
the commit message say xenvif_get_stats() instead?

> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index e020f44ac2c71..d1c9e38cfb8ef 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -835,10 +835,21 @@ static void connect(struct backend_info *be)
>  	return;
>  
>  err:
> -	if (be->vif->num_queues > 0)
> +	if (be->vif->num_queues > 0) {
> +		unsigned int num_queues = be->vif->num_queues;
> +
>  		xenvif_disconnect_data(be->vif); /* Clean up existing queues */
> -	for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
> -		xenvif_deinit_queue(&be->vif->queues[queue_index]);
> +
> +		/*
> +		 * Mirror backend_disconnect(): publish num_queues == 0 and
> +		 * synchronize_net() so RCU stats readers finish before the free.
> +		 */
> +		be->vif->num_queues = 0;
> +		synchronize_net();
> +
> +		for (queue_index = 0; queue_index < num_queues; ++queue_index)
> +			xenvif_deinit_queue(&be->vif->queues[queue_index]);
> +	}
>  	vfree(be->vif->queues);

[Severity: High]
Is the use-after-free still open when the very first queue fails?

Earlier in connect(), num_queues is published before any queue is set up:

	be->vif->num_queues = requested_num_queues;
	be->vif->stalled_queues = requested_num_queues;

If xenvif_init_queue() or connect_data_rings() then fails for
queue_index 0, the failure branch resets the count. For example, when
connect_data_rings() fails:

	xenvif_deinit_queue(queue);
	be->vif->num_queues = queue_index;
	goto err;

This sets num_queues to 0, so the new if (be->vif->num_queues > 0) block
is skipped. synchronize_net() never runs, and vfree(be->vif->queues)
happens with no grace period.

A reader that sampled a non-zero num_queues during the
connect_data_rings() window could still be inside rcu_read_lock():

xenvif_get_stats() {
	rcu_read_lock();
	num_queues = READ_ONCE(vif->num_queues);
	for (index = 0; index < num_queues; ++index) {
		queue = &vif->queues[index];
	...
}

xenvif_get_ethtool_stats() follows the same pattern.

multi-queue-num-queues defaults to 1, and with one queue every failure
happens at queue_index 0. So for a default single-queue frontend, a guest
that supplies a bad ring reference or event channel would still reach
this vfree() without a grace period.

The new comment says this mirrors backend_disconnect(). However,
backend_disconnect() calls synchronize_net() before vfree(vif->queues)
whatever the queue count is.

The commit message says connect() jumps to err: "with num_queues still
counting the queues already set up". That doesn't hold in the queue 0
case.

Should the grace period run whenever vif->queues was allocated and
num_queues was published, and not only when num_queues > 0 at the err
label?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027121438.1266207-1-yhlee%40isslab.korea.ac.kr