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

[sashiko-bot@kernel.org]

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