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 <[email protected]> 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/[email protected]?part=1
