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.
Also reset the device before del_vqs() on these paths, like
remove_common() and the virtio_net/virtio_scsi error unwinds do:
without the reset the queues stay enabled on the device and the next
probe fails in setup_vq() with -ENOENT.
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: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <[email protected]>
---
The wakeup_lock dependency is now in mainline (161aa708b02c).
Changes in v4 (from sashiko review):
- reset the device before del_vqs() on both error paths, like
remove_common() and the virtio_net/virtio_scsi unwinds: without the
reset queue_enable stays set and the next probe fails in setup_vq()
with -ENOENT.
drivers/virtio/virtio_balloon.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec1..b30a0ca26 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,9 @@ static int init_vqs(struct virtio_balloon *vb)
if (err) {
dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
__func__);
+ virtballoon_stop_updates(vb);
+ virtio_reset_device(vb->vdev);
+ vb->vdev->config->del_vqs(vb->vdev);
return err;
}
virtqueue_kick(vb->stats_vq);
@@ -1075,6 +1089,8 @@ 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:
+ virtballoon_stop_updates(vb);
+ virtio_reset_device(vdev);
vdev->config->del_vqs(vdev);
out_free_vb:
mutex_destroy(&vb->balloon_lock);
@@ -1115,11 +1131,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
