Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] block: NULL pointer dereference in virtblk_ctrl_vq_request on device removal - [High] block: memory leak in virtblk_ctrl_vq_request due to timeout race - [High] block: UAF and double free in virtblk_probe error path - [Low] block: missing mutex_destroy on virtblk_probe error path --
commit 4ba5fa6213a4ecb3d12630534388c763dd962458 Author: Linlin Zhang <[email protected]> Subject: virtio_blk: Add control virtqueue support This patch adds support for the optional virtio-blk control virtqueue. It allows the driver to manage control-queue requests independently from the data path, preparing for a subsequent inline encryption implementation. > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index c8bfbf8435ac7..2499ae2fb9e22 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c [ ... ] > @@ -864,11 +898,181 @@ static int virtblk_getgeo(struct gendisk *disk, struct > hd_geometry *geo) > +static int virtblk_ctrl_vq_request(struct virtio_blk *vblk, > + struct virtblk_ctrl_request *creq, > + struct scatterlist *sgs[], > + unsigned int out_sgs, unsigned int in_sgs) > +{ [ ... ] > + if (wait_for_completion_timeout(&comp->done, VIRTBLK_CTRL_VQ_TIMEOUT)) { > + mutex_unlock(&vblk->ctrl_vq.mutex); > + kfree(comp); > + creq->compl = NULL; > + return 0; > + } > + > + /* > + * The host hasn't responded within the timeout. @creq is still > + * owned by the device, so don't touch its DMA-target fields or > + * free it here. Mark it abandoned and hand ownership of both @creq > + * and @comp to whichever of virtblk_ctrlq_callback() or > + * virtblk_ctrl_vq_drain() retrieves the buffer later; unlock the > + * mutex so subsequent requests aren't serialized behind an > + * unresponsive host. > + */ > + spin_lock_irqsave(&vblk->ctrl_vq.lock, flags); > + comp->abandoned = true; > + spin_unlock_irqrestore(&vblk->ctrl_vq.lock, flags); > + mutex_unlock(&vblk->ctrl_vq.mutex); [Severity: High] Could this leak memory if the request completes just as the timeout expires? When wait_for_completion_timeout() expires, the waiter acquires ctrl_vq.lock and sets comp->abandoned = true without checking if the completion was already signaled. If the hardware interrupt fires concurrently, virtblk_ctrlq_callback() might run on another CPU before the lock is acquired, read abandoned as false, call complete(), and skip freeing the memory. Since both paths assume the other is responsible for cleanup during this specific race, the creq and comp structures would be leaked permanently. > + > + dev_warn(&vblk->vdev->dev, > + "control queue request timed out, abandoning\n"); [Severity: Critical] Is it safe to dereference vblk->vdev here without holding vdev_mutex? If a request times out exactly as the device is being removed, virtblk_remove() calls virtblk_ctrl_vq_quiesce(), which waits for the same 10-second timeout. When quiesce times out, it proceeds to reset the device, drain the queue, and set vblk->vdev = NULL. Meanwhile, this thread wakes up from the timeout, drops ctrl_vq.mutex (which does not hold vdev_mutex), and dereferences vblk->vdev to log the warning. Since vblk->vdev might have been cleared by the concurrent teardown, this could cause a NULL pointer dereference. > + return -ETIMEDOUT; > +} [ ... ] > @@ -1465,14 +1686,18 @@ static int virtblk_probe(struct virtio_device *vdev) > } > > mutex_init(&vblk->vdev_mutex); > + mutex_init(&vblk->ctrl_vq.mutex); > + spin_lock_init(&vblk->ctrl_vq.lock); > > vblk->vdev = vdev; > > INIT_WORK(&vblk->config_work, virtblk_config_changed_work); > > err = init_vq(vblk); > - if (err) > + if (err) { > + dev_err(&vdev->dev, "init virt queue failed: err = %d\n", err); > goto out_free_vblk; > + } [Severity: Low] Does this error path miss destroying the newly initialized mutex? mutex_init(&vblk->ctrl_vq.mutex) happens early in the probe sequence. If init_vq() fails, the code jumps to out_free_vblk, which only calls kfree(vblk). This bypasses mutex_destroy(), potentially leaking lockdep tracking state when CONFIG_DEBUG_MUTEXES is enabled. [ ... ] > @@ -1554,6 +1779,7 @@ static int virtblk_probe(struct virtio_device *vdev) > out_free_vq: > vdev->config->del_vqs(vdev); > kfree(vblk->vqs); > + vblk->ctrl_vq.vq = NULL; > out_free_vblk: > kfree(vblk); [Severity: High] Could this error path trigger a use-after-free and double free? If an error occurs and the path jumps to out_cleanup_disk (just above this diff snippet), the code calls put_disk(vblk->disk). Since the disk has a refcount of 1 here, this drops it to 0, triggering the virtblk_free_disk() callback which frees vblk. Execution then falls through to blk_mq_free_tag_set(&vblk->tag_set), touches the freed memory by assigning vblk->ctrl_vq.vq = NULL, and ultimately calls kfree(vblk) again. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
