Re: [PATCH v4 2/2] virtio_blk: add inline encryption support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] block: 3-way deadlock between vdev_mutex, q_usage_counter, and queue freeze
- [High] block: virtio-blk mempool-less dynamic allocation in bio submission path
- [Medium] virtio_blk: TOCTOU race in ctrl_vq timeout causes memory leak
- [Medium] block: memory leak of crypto profile on virtblk_probe error path
--

commit d627f18e8794bdd76b3e4d25da68c27a6fd40f71
Author: Linlin Zhang <linlin.zhang@oss.qualcomm.com>

virtio_blk: add inline encryption support

This commit adds support for the virtio-blk inline encryption feature. It
allows offloading per-I/O encryption to the host's inline crypto engine
instead of performing it in software in the guest.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2499ae2fb9e22..6fd23b45868a1 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
[ ... ]
> @@ -985,7 +1051,7 @@ static void virtblk_ctrlq_callback(struct virtqueue *vq)
>  			 */
>  			if (abandoned) {
>  				kfree(creq->compl);
> -				kfree(creq);
> +				kfree_sensitive(creq);
>  			} else {
>  				complete(&creq->compl->done);
>  			}

[Severity: Medium]
Does this modification leave a time-of-check to time-of-use (TOCTOU) memory
leak unaddressed?

If a timeout expires in virtblk_ctrl_vq_request(), the waiter thread prepares
to mark the request abandoned. If the device interrupt fires concurrently and
virtblk_ctrlq_callback() reads abandoned as false before the waiter updates
it, the callback will complete the request without freeing it.

The waiter then sets abandoned to true and returns -ETIMEDOUT. Callers like
virtblk_crypto_keyslot_program() receive -ETIMEDOUT and skip their local
cleanup, assuming the background mechanism will free it. Neither path frees
the memory.

[ ... ]
> @@ -1067,11 +1133,566 @@ static int virtblk_ctrl_vq_request(struct virtio_blk *vblk,
[ ... ]
> +static int virtblk_crypto_keyslot_program(struct blk_crypto_profile *profile,
> +					   const struct blk_crypto_key *key,
> +					   unsigned int slot)
> +{
> +	struct virtio_blk *vblk = virtblk_from_profile(profile);
> +	struct scatterlist type_sg, out_req_sg, status_sg, *sgs[3];
> +	struct virtblk_ctrl_request *creq;
> +	int err;
> +
> +	mutex_lock(&vblk->vdev_mutex);

[Severity: High]
Can this create an AB-BA-C deadlock with queue freezing?

virtblk_crypto_keyslot_program() acquires vdev_mutex. Since this runs as a
block crypto profile callback during bio submission, the q_usage_counter read
reference is already held by the submitting thread.

If a concurrent teardown initiates a queue freeze via blk_mq_freeze_queue(),
it blocks waiting for existing readers to finish. Concurrently, if
virtblk_report_zones() has acquired vdev_mutex and attempts to allocate a
request (blocking on the freeze), a 3-way deadlock occurs where
virtblk_report_zones blocks on the freeze, the freeze blocks on the bio
submission, and the bio submission blocks on vdev_mutex.

> +	if (!vblk->vdev) {
> +		err = -ENXIO;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * GFP_NOIO: this callback runs on the bio-submission path, which
> +	 * memory reclaim can reach while writing back dirty pages to this
> +	 * same device; GFP_KERNEL here could recurse into that same reclaim
> +	 * and self-deadlock.
> +	 */
> +	creq = kzalloc_obj(*creq, GFP_NOIO);

[Severity: High]
Is it safe to use a mempool-less dynamic allocation here in the synchronous
bio submission path?

Under severe memory pressure, kzalloc_obj() (and kmalloc_obj() in
virtblk_ctrl_vq_request) can fail and return -ENOMEM. This causes the block
layer to immediately fail the bio with BLK_STS_RESOURCE instead of requeuing
it, potentially dropping page writebacks and breaking forward-progress
guarantees.

[ ... ]
> @@ -1764,6 +2387,28 @@ static int virtblk_probe(struct virtio_device *vdev)
[ ... ]
> +		} else {
> +			err = virtblk_init_crypto(vblk);
> +			if (!err) {
> +				if (!blk_crypto_register(&vblk->profile, vblk->disk->queue))
> +					dev_warn(&vdev->dev,
> +						"failed to register inline crypto profile\n");
> +			} else {
> +				dev_warn(&vdev->dev,
> +					"inline crypto init failed: %d, continuing without inline crypto support\n",
> +					err);
> +			}
> +		}
>  	}
>  
>  	err = device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);

[Severity: Medium]
Does this error path leak the crypto profile allocations?

If device_add_disk() fails, the probe function jumps to out_cleanup_disk.
Because device_add_disk() failed, the disk's GD_ADDED flag isn't set,
meaning put_disk() will skip the .free_disk callback (virtblk_free_disk).
As a result, virtblk_destroy_crypto() is never called, leaking the
dynamically allocated profile.slots and profile.slot_hashtable arrays.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009041727.3170811-1-linlin.zhang@oss.qualcomm.com?part=2