[PATCH] virtio_balloon: drain update works on probe error paths

[PATCH] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---
 drivers/virtio/virtio_balloon.c | 17 ++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -554,6 +554,17 @@ static void update_balloon_stats_func(struct work_struct *work)
 	finish_wakeup_event(vb);
 }

+/* Stop queueing of and drain any pending balloon update works. */
+static void virtballoon_stop_updates(struct virtio_balloon *vb)
+{
+	spin_lock_irq(&vb->stop_update_lock);
+	vb->stop_update = true;
+	spin_unlock_irq(&vb->stop_update_lock);
+
+	cancel_work_sync(&vb->update_balloon_size_work);
+	cancel_work_sync(&vb->update_balloon_stats_work);
+}
+
 static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
@@ -1076,6 +1087,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		destroy_workqueue(vb->balloon_wq);
 out_del_vqs:
+	/*
+	 * init_vqs() arms the stats virtqueue callback and primes the
+	 * queue, so an update work may already be pending: stop queueing
+	 * and drain it before the virtqueues and vb go away.
+	 */
+	virtballoon_stop_updates(vb);
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	mutex_destroy(&vb->balloon_lock);
@@ -1116,11 +1133,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(vb);

-	spin_lock_irq(&vb->stop_update_lock);
-	vb->stop_update = true;
-	spin_unlock_irq(&vb->stop_update_lock);
-	cancel_work_sync(&vb->update_balloon_size_work);
-	cancel_work_sync(&vb->update_balloon_stats_work);
+	virtballoon_stop_updates(vb);

 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
--
2.47.0

Re: [PATCH] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] virtio_balloon: uninitialized wakeup_lock used in virtballoon_probe()
- [High] virtio_balloon: leak vqs and UAF vb when init_vqs() fails after find_vqs
--

commit ead8a642cbbc78b91d17dc13d18353f2a7792fd1
Author: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

virtio_balloon: drain update works on probe error paths

This commit factors out virtballoon_stop_updates() from virtballoon_quiesce()
and calls it on the virtballoon_probe() error path before deleting the
virtqueues and freeing struct virtio_balloon.

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..caac7f5b3459c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -554,6 +554,17 @@ static void update_balloon_stats_func(struct work_struct *work)
>  	finish_wakeup_event(vb);
>  }
>  
> +/* Stop queueing of and drain any pending balloon update works. */
> +static void virtballoon_stop_updates(struct virtio_balloon *vb)
> +{
> +	spin_lock_irq(&vb->stop_update_lock);
> +	vb->stop_update = true;
> +	spin_unlock_irq(&vb->stop_update_lock);
> +
> +	cancel_work_sync(&vb->update_balloon_size_work);
> +	cancel_work_sync(&vb->update_balloon_stats_work);
> +}
> +
>  static void update_balloon_size_func(struct work_struct *work)
>  {
>  	struct virtio_balloon *vb;
> @@ -1076,6 +1087,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>  		destroy_workqueue(vb->balloon_wq);
>  out_del_vqs:
> +	/*
> +	 * init_vqs() arms the stats virtqueue callback and primes the
> +	 * queue, so an update work may already be pending: stop queueing
> +	 * and drain it before the virtqueues and vb go away.
> +	 */
> +	virtballoon_stop_updates(vb);
>  	vdev->config->del_vqs(vdev);
>  out_free_vb:
>  	mutex_destroy(&vb->balloon_lock);

[Severity: High]
Can this acquire an uninitialized vb->wakeup_lock when stats_request() fires
during virtballoon_probe()?

In virtballoon_probe(), init_vqs(vb) arms stats_request() and kicks stats_vq
early in the function, whereas spin_lock_init(&vb->wakeup_lock) is only called
near the end of virtballoon_probe(), after all of the error jumps that unwind
through out_del_vqs:

drivers/virtio/virtio_balloon.c:virtballoon_probe() {
	...
	spin_lock_init(&vb->stop_update_lock);
	mutex_init(&vb->balloon_lock);
	init_waitqueue_head(&vb->acked);
	vb->vdev = vdev;

	balloon_devinfo_init(&vb->vb_dev_info);

	err = init_vqs(vb);
	if (err)
		goto out_free_vb;
	...
		err = page_reporting_register(&vb->pr_dev_info);
		if (err)
			goto out_unregister_oom;
	}

	spin_lock_init(&vb->wakeup_lock);
}

When stats_request() triggers during virtballoon_probe(), it calls
start_wakeup_event(vb, ...) -> spin_lock_irqsave(&vb->wakeup_lock, flags)
before vb->wakeup_lock has been initialized.

If a subsequent probe step fails and jumps to out_del_vqs,
spin_lock_init(&vb->wakeup_lock) is skipped altogether, and draining
update_balloon_stats_work in virtballoon_stop_updates(vb) runs:

update_balloon_stats_func()
  -> process_wakeup_event() -> spin_lock_irq(&vb->wakeup_lock)
  -> finish_wakeup_event()  -> spin_lock_irq(&vb->wakeup_lock)

on an uninitialized spinlock. Even when probe succeeds, if stats_request()
fires before spin_lock_init(&vb->wakeup_lock), spin_lock_init() will
re-initialize vb->wakeup_lock while it may already be in use. Should
spin_lock_init(&vb->wakeup_lock) be moved before init_vqs(vb)?

[Severity: High]
Also in virtballoon_probe(), what happens if init_vqs() fails at
virtqueue_add_outbuf() after virtio_find_vqs() has already succeeded?

drivers/virtio/virtio_balloon.c:init_vqs() {
	...
	err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
			      vqs_info, NULL);
	if (err)
		return err;
	...
	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
		...
		err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
					   GFP_KERNEL);
		if (err) {
			dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
				 __func__);
			return err;
		}
		virtqueue_kick(vb->stats_vq);
	}
}

If virtqueue_add_outbuf() fails (for example, on a DMA mapping failure),
init_vqs() returns an error while the virtqueues allocated by
virtio_find_vqs() and their callbacks remain active. In virtballoon_probe(),
the error path for init_vqs() jumps directly to out_free_vb, bypassing
out_del_vqs:

	err = init_vqs(vb);
	if (err)
		goto out_free_vb;

Could this leak the virtqueues and leave the armed callbacks and any queued
update_balloon_stats_work referencing vb after kfree(vb)?

[ ... ]

>
>
>

[]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019084406.162-1-zhaimingbing@cmss.chinamobile.com?part=1

[PATCH v2] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

The same hole exists when init_vqs() itself fails after
virtio_find_vqs() succeeded: the stats priming can fail with the queues
up and the callback armed, yet probe jumps straight to out_free_vb and
leaks the virtqueues while the queued work still references vb.  Unwind
inside init_vqs() in that case, stopping the works before the queues
are deleted.

The drained work takes vb->wakeup_lock in process_wakeup_event() and
finish_wakeup_event(), so this depends on "virtio_balloon: initialize
wakeup_lock before virtqueues are set up", which moves that
spin_lock_init() to the top of probe.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---

Changes in v2 (from sashiko review):
- unwind inside init_vqs() when the stats priming fails after
  virtio_find_vqs() succeeded, instead of leaking the virtqueues on the
  out_free_vb path with the callback still armed.
- spell out the dependency on the wakeup_lock initialization patch:
  the drained work takes that lock.

 drivers/virtio/virtio_balloon.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec1..dd2056882 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -554,6 +554,17 @@ static void update_balloon_stats_func(struct work_struct *work)
 	finish_wakeup_event(vb);
 }
 
+/* Stop queueing of and drain any pending balloon update works. */
+static void virtballoon_stop_updates(struct virtio_balloon *vb)
+{
+	spin_lock_irq(&vb->stop_update_lock);
+	vb->stop_update = true;
+	spin_unlock_irq(&vb->stop_update_lock);
+
+	cancel_work_sync(&vb->update_balloon_size_work);
+	cancel_work_sync(&vb->update_balloon_stats_work);
+}
+
 static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
@@ -633,6 +644,12 @@ static int init_vqs(struct virtio_balloon *vb)
 		if (err) {
 			dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
 				 __func__);
+			/*
+			 * The virtqueues are up and the stats callback is
+			 * armed: stop the works and unwind the queues.
+			 */
+			virtballoon_stop_updates(vb);
+			vb->vdev->config->del_vqs(vb->vdev);
 			return err;
 		}
 		virtqueue_kick(vb->stats_vq);
@@ -1075,6 +1092,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		destroy_workqueue(vb->balloon_wq);
 out_del_vqs:
+	/*
+	 * init_vqs() arms the stats virtqueue callback and primes the
+	 * queue, so an update work may already be pending: stop queueing
+	 * and drain it before the virtqueues and vb go away.
+	 */
+	virtballoon_stop_updates(vb);
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	mutex_destroy(&vb->balloon_lock);
@@ -1115,11 +1138,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(vb);
 
-	spin_lock_irq(&vb->stop_update_lock);
-	vb->stop_update = true;
-	spin_unlock_irq(&vb->stop_update_lock);
-	cancel_work_sync(&vb->update_balloon_size_work);
-	cancel_work_sync(&vb->update_balloon_stats_work);
+	virtballoon_stop_updates(vb);
 
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
-- 
2.43.0

Re: [PATCH v2] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] virtio_balloon: uninitialized wakeup_lock used in virtballoon_stop_updates()
- [High] virtio_balloon: missing virtio_reset_device() before del_vqs() on probe error
- [High] virtio_balloon: race between init_vqs() and stats_handle_request()
--

commit e212fced31f70fa27727822aaf449978f76f3a04
Author: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

virtio_balloon: drain update works on probe error paths

This commit factors out virtballoon_stop_updates() from virtballoon_quiesce()
and calls it on the virtballoon_probe() and init_vqs() error paths to stop
and drain pending balloon update works before deleting virtqueues and freeing
the virtio_balloon structure.

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..fe99ab0fea227 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -554,6 +554,17 @@ static void update_balloon_stats_func(struct work_struct *work)
>  	finish_wakeup_event(vb);
>  }
>  
> +/* Stop queueing of and drain any pending balloon update works. */
> +static void virtballoon_stop_updates(struct virtio_balloon *vb)
> +{
> +	spin_lock_irq(&vb->stop_update_lock);
> +	vb->stop_update = true;
> +	spin_unlock_irq(&vb->stop_update_lock);
> +
> +	cancel_work_sync(&vb->update_balloon_size_work);
> +	cancel_work_sync(&vb->update_balloon_stats_work);
> +}

[Severity: High]
Does draining vb->update_balloon_stats_work via virtballoon_stop_updates()
on the init_vqs() and virtballoon_probe() error paths acquire an
uninitialized vb->wakeup_lock?

In virtballoon_probe(), init_vqs(vb) is called near the beginning of probe,
while spin_lock_init(&vb->wakeup_lock) is not called until near the end of
virtballoon_probe(), after init_vqs() and after all error jumps to
out_del_vqs, out_del_balloon_wq, out_unregister_shrinker, and
out_unregister_oom:

drivers/virtio/virtio_balloon.c:virtballoon_probe() {
	...
	err = init_vqs(vb);
	if (err)
		goto out_free_vb;
	...
		err = page_reporting_register(&vb->pr_dev_info);
		if (err)
			goto out_unregister_oom;
	}

	spin_lock_init(&vb->wakeup_lock);
	...
}

Once init_vqs() calls virtio_find_vqs(), the stats_vq callback
stats_request() is armed and calls start_wakeup_event(), which executes
spin_lock_irqsave(&vb->wakeup_lock, flags) on the uninitialized lock. If
init_vqs() fails at virtqueue_add_outbuf() or a later step in
virtballoon_probe() fails and jumps to out_del_vqs,
virtballoon_stop_updates(vb) drains update_balloon_stats_func(), which calls
process_wakeup_event() and finish_wakeup_event() and executes
spin_lock_irq(&vb->wakeup_lock) without spin_lock_init(&vb->wakeup_lock)
ever having been called. Furthermore, if stats_request() fires during or
after init_vqs() and probe succeeds, spin_lock_init(&vb->wakeup_lock) will
re-initialize the lock while stats_request() or update_balloon_stats_func()
may already be holding or contending on it.

Although the commit message notes a dependency on "virtio_balloon:
initialize wakeup_lock before virtqueues are set up", that change is not
present in the tree or included in this commit. Should
spin_lock_init(&vb->wakeup_lock) be moved before init_vqs(vb) here so that
vb->wakeup_lock is initialized before the virtqueue callbacks are armed and
before virtballoon_stop_updates() can drain the work?

>  
>  static void update_balloon_size_func(struct work_struct *work)
>  {
>  	struct virtio_balloon *vb;
> @@ -633,6 +644,12 @@ static int init_vqs(struct virtio_balloon *vb)
>  		if (err) {
>  			dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
>  				 __func__);
> +			/*
> +			 * The virtqueues are up and the stats callback is
> +			 * armed: stop the works and unwind the queues.
> +			 */
> +			virtballoon_stop_updates(vb);
> +			vb->vdev->config->del_vqs(vb->vdev);
>  			return err;
>  		}
>  		virtqueue_kick(vb->stats_vq);

[Severity: High]
Can stats_request() and stats_handle_request() race with init_vqs() as soon
as virtio_find_vqs() returns?

In init_vqs(), virtio_find_vqs() sets up the virtqueues and arms the
stats_vq callback stats_request() while vb->stop_update is false, before
vb->stats_vq is assigned and before initial priming finishes:

drivers/virtio/virtio_balloon.c:init_vqs() {
	...
	err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
			      vqs_info, NULL);
	if (err)
		return err;

	vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
	vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
		struct scatterlist sg;
		unsigned int num_stats;
		vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS];

		num_stats = update_balloon_stats(vb);

		sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
		err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
					   GFP_KERNEL);
	...
}

If the device triggers stats_request() and update_balloon_stats_func() ->
stats_handle_request() runs on another CPU before vb->stats_vq =
vqs[VIRTIO_BALLOON_VQ_STATS] is assigned, stats_handle_request() will read
vq = vb->stats_vq as NULL and pass it to virtqueue_get_buf(vq, &len):

drivers/virtio/virtio_balloon.c:stats_handle_request() {
	...
	num_stats = update_balloon_stats(vb);

	vq = vb->stats_vq;
	if (!virtqueue_get_buf(vq, &len))
		return;
	sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
	err = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
	if (WARN_ON_ONCE(err))
		return;
	virtqueue_kick(vq);
}

Even after vb->stats_vq is assigned, if stats_handle_request() runs
concurrently with the priming in init_vqs(), both paths will concurrently
write to vb->stats[] in update_balloon_stats(vb) and invoke unserialized
virtqueue operations (virtqueue_get_buf(), virtqueue_add_outbuf(),
virtqueue_kick()) on vb->stats_vq without any lock.

> @@ -1076,6 +1093,12 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>  		destroy_workqueue(vb->balloon_wq);
>  out_del_vqs:
> +	/*
> +	 * init_vqs() arms the stats virtqueue callback and primes the
> +	 * queue, so an update work may already be pending: stop queueing
> +	 * and drain it before the virtqueues and vb go away.
> +	 */
> +	virtballoon_stop_updates(vb);
>  	vdev->config->del_vqs(vdev);
>  out_free_vb:
>  	mutex_destroy(&vb->balloon_lock);

[Severity: High]
Should virtio_reset_device(vdev) be called before vdev->config->del_vqs(vdev)
here at out_del_vqs (and on the error path in init_vqs())?

In init_vqs(), virtio_find_vqs() enables the virtqueues and arms IRQ
callbacks, and when VIRTIO_BALLOON_F_STATS_VQ is negotiated, init_vqs()
adds vb->stats as an outbuf to vb->stats_vq via virtqueue_add_outbuf() and
kicks vb->stats_vq. In addition, if update_balloon_stats_func() was running
when virtballoon_stop_updates(vb) drained it, stats_handle_request() will
have re-added vb->stats to vb->stats_vq and kicked the queue again.

Unlike remove_common(), which resets the device before deleting the
virtqueues:

drivers/virtio/virtio_balloon.c:remove_common() {
	...
	/* Now we reset the device so we can clean up the queues. */
	virtio_reset_device(vb->vdev);

	vb->vdev->config->del_vqs(vb->vdev);
}

neither out_del_vqs nor init_vqs() calls virtio_reset_device() before
calling del_vqs() and kfree(vb) (and virtio_dev_probe() only sets
VIRTIO_CONFIG_S_FAILED on error without resetting the device).

Without virtio_reset_device(vdev), on modern virtio-pci (vp_modern_del_vqs())
enabled queues are not disabled without a reset, leaving the device active
with DMA mappings to the freed vring pages and to vb->stats after kfree(vb).
On virtio-mmio (vm_del_vqs()), vring_del_virtqueue() deletes and frees each
virtqueue from vdev->vqs before free_irq() is called, so without
virtio_reset_device() to stop the device and synchronize callbacks,
vm_interrupt() can concurrently iterate vdev->vqs and dereference freed
virtqueue structures.

[ ... ]

>
>
>

[]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020035744.1131-1-zhaimingbing@cmss.chinamobile.com?part=1