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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:

New issues:
- [Critical] REQ_OP_ZONE_APPEND requests with an encryption context are silently written in plaintext.
- [High] Zoned block devices completely skip inline encryption initialization due to an 'else if' block.
- [High] Memory allocation on the block I/O submission path uses GFP_KERNEL without a mempool, causing deadlocks.
- [High] Hardware queues are unquiesced before encryption keys are reprogrammed during resume.
- [High] Cacheline sharing in `struct virtblk_ctrl_request` violates the DMA API, risking memory corruption.
- [High] Uninitialized vblk->crypto_profile_initialized leads to wild frees on probe error paths.
- [High] blk-crypto operations can race with device removal, leading to NULL pointer dereference.
- [Medium] The driver directly assigns Linux-internal crypto mode enums to the Virtio hardware descriptor without translation.
- [Low] VIRTIO_BLK_INLINE_ENCRYPTION is defined as a tristate but cannot be built as an independent module.

Pre-existing issues:
- [High] The error cleanup path in virtblk_probe contains a use-after-free and double free of vblk.
--

commit 787711d9770fce24131418d43dc33cb33a894de6
Author: linlzhan <linlin.zhang@oss.qualcomm.com>

virtio_blk: add inline encryption support

This commit adds support for the virtio-blk inline encryption feature,
allowing guest VMs to offload per-I/O encryption to the host's inline
crypto engine. It wires up device encryption characteristics to a
struct blk_crypto_profile and handles key management via control commands.

> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 858320b6ebb7e..58bb050d4617c 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -372,4 +372,16 @@ config BLK_DEV_ZONED_LOOP
>  
>  	  If unsure, say N.
>  
> +config VIRTIO_BLK_INLINE_ENCRYPTION
> +	tristate "Virtio block inline encryption support"
> +	depends on VIRTIO_BLK && BLK_INLINE_ENCRYPTION

[Severity: Low]
Could this lead to configuration ambiguity? VIRTIO_BLK_INLINE_ENCRYPTION is
defined as a tristate, but the code is conditionally included directly within
the virtio_blk module. Users might select 'M' thinking it builds a separate
crypto module, but it functions as an intra-file boolean.

[ ... ]

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 252ec93661532..3fd318e5e1f17 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -124,8 +136,23 @@ struct virtblk_req {
>  };
>  
>  struct virtblk_ctrl_request {
> +	/* Type byte, always its own out-sg for every command. */
>  	__virtio32 type;
> +	/* Out request, sent as a second, separate out-sg if any. */
> +	union {
> +		struct virtio_blk_crypto_key_desc key_desc;
> +		struct virtio_blk_crypto_key_blob blob;
> +	} out_req;
> +
> +	/* In response */
> +	union {
> +		struct virtio_blk_crypto_key_blob blob;
> +		struct virtio_blk_crypto_sw_secret secret;
> +		struct virtio_blk_crypto_modes modes;
> +	} in_resp;
> +	/* Status byte, always its own in-sg for every command. */
>  	u8 status;
> +
>  	struct completion compl;
>  };

[Severity: High]
Does this struct packing risk memory corruption on non-cache-coherent
architectures?

The DMA-written buffers (in_resp, status) are packed directly adjacent to the
compl field. On non-coherent architectures, cache invalidation for the DMA
fields can destroy the CPU's ongoing modifications to the waitqueue inside
compl during wait_for_completion().

[ ... ]

> @@ -262,20 +310,27 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  				      struct request *req,
>  				      struct virtblk_req *vbr)
>  {
[ ... ]
>  	case REQ_OP_ZONE_FINISH:
>  		type = VIRTIO_BLK_T_ZONE_FINISH;
>  		sector = blk_rq_pos(req);
>  		break;
>  	case REQ_OP_ZONE_APPEND:
>  		type = VIRTIO_BLK_T_ZONE_APPEND;
>  		sector = blk_rq_pos(req);
>  		in_hdr_len = sizeof(vbr->in_hdr.zone_append);
>  		break;

[Severity: Critical]
Could this lead to plaintext data being written for encrypted zone append
requests?

When handling REQ_OP_ZONE_APPEND, the type is forced to
VIRTIO_BLK_T_ZONE_APPEND. Because this path does not check is_crypto_request(),
it bypasses the crypto setup...

[ ... ]

> @@ -337,6 +392,18 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
>  	    type == VIRTIO_BLK_T_SECURE_ERASE) {
>  		if (virtblk_setup_discard_write_zeroes_erase(req, unmap))
>  			return BLK_STS_RESOURCE;
>  	}
>  
> +	if (type == VIRTIO_BLK_T_CRYPTO_IN || type == VIRTIO_BLK_T_CRYPTO_OUT) {
> +		memset(&vbr->out_hdr.crypto_append.msg, 0,

[Severity: Critical]
...and subsequently fails this check since type is VIRTIO_BLK_T_ZONE_APPEND
instead of VIRTIO_BLK_T_CRYPTO_OUT.

This prevents the crypto message descriptor from being attached, causing the
host hypervisor to see a standard unencrypted write and store the plaintext
data to disk.

[ ... ]

> @@ -1001,11 +1068,478 @@ static int virtblk_ctrl_vq_request(struct virtio_blk *vblk,
[ ... ]
> +static int set_virtblk_crypto_key_desc(struct virtio_device *vdev,
> +				       struct virtblk_ctrl_request *creq,
> +				       const struct blk_crypto_key *key,
> +				       unsigned int slot)
> +{
> +	struct virtio_blk_crypto_key_desc *desc = &creq->out_req.key_desc;
> +	enum blk_crypto_key_type key_type = key->crypto_cfg.key_type;
> +
> +	if (sizeof(desc->bytes) < key->size)
> +		return -EOVERFLOW;
> +
> +	memset(desc, 0, sizeof(*desc));
> +	desc->slot = cpu_to_virtio32(vdev, slot);
> +	memcpy(desc->bytes, key->bytes, key->size);
> +	desc->key_size = cpu_to_virtio32(vdev, key->size);
> +	desc->crypto_mode = cpu_to_virtio32(vdev, key->crypto_cfg.crypto_mode);

[Severity: Medium]
Can this leak a Linux-internal enum value directly into the hardware protocol
descriptor?

The desc->crypto_mode is set directly from key->crypto_cfg.crypto_mode, which
is an internal enum blk_crypto_mode_num. If Linux adds a new mode or virtio
assigns different numbers in the future, this will silently send incorrect
crypto modes to the hypervisor.

[ ... ]

> +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;
> +
> +	creq = kzalloc_obj(*creq, GFP_KERNEL);
> +	if (!creq)
> +		return -ENOMEM;

[Severity: High]
Is it safe to use GFP_KERNEL here?

This allocation occurs on the block I/O submission path. Under memory pressure,
this could recurse into memory reclaim, which may attempt to flush dirty pages
to this same block device, leading to a deadlock.

[ ... ]

> +static int virtblk_crypto_keyslot_evict(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;
> +
> +	creq = kzalloc_obj(*creq, GFP_KERNEL);
> +	if (!creq)
> +		return -ENOMEM;

[Severity: High]
Similar to the programming path, is GFP_KERNEL appropriate here? The eviction
path often occurs during inode or dentry reclaim, and recursing into memory
reclaim could cause deadlocks.

> +
> +	creq->type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_CRYPTO_KEYSLOT_EVICT);

[Severity: High]
Could this race with device removal?

In virtblk_crypto_keyslot_evict(), vblk->vdev is accessed unconditionally. If
the virtio device is hot-unplugged, virtblk_remove() sets vblk->vdev to NULL.
Since the block device may still be open by userspace, fscrypt might
subsequently tear down a key and call into this driver, causing a NULL pointer
dereference.

[ ... ]

> +static void virtblk_destroy_crypto(struct virtio_blk *vblk)
> +{
> +	if (vblk->crypto_profile_initialized)
> +		blk_crypto_profile_destroy(&vblk->profile);
> +}

[Severity: High]
Could this cause a wild free on probe error paths?

vblk is allocated using kmalloc_obj() which does not zero memory. If the probe
fails before the profile is initialized, virtblk_destroy_crypto() might read a
true value for vblk->crypto_profile_initialized and pass garbage to
blk_crypto_profile_destroy().

[ ... ]

> @@ -1698,6 +2232,19 @@ static int virtblk_probe(struct virtio_device *vdev)
[ ... ]
>  	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>  	    (lim.features & BLK_FEAT_ZONED)) {
>  		err = blk_revalidate_disk_zones(vblk->disk);
>  		if (err)
>  			goto out_cleanup_disk;
> +	} else if (IS_ENABLED(CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION) &&
> +		   virtio_has_feature(vdev, VIRTIO_BLK_F_INLINE_ENCRYPTION) &&
> +		   virtio_has_feature(vdev, VIRTIO_BLK_F_CTRL_VQ)) {
> +		err = virtblk_init_crypto(vblk);

[Severity: High]
Does this else if inadvertently disable inline encryption for zoned block
devices?

If a device advertises both ZONED and INLINE_ENCRYPTION features, the initial
if block executes, causing the entire crypto initialization block to be skipped
silently.

[ ... ]

>  	err = device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
>  	if (err)
>  		goto out_cleanup_disk;
>  
>  	return 0;
>  
>  out_cleanup_disk:
>  	put_disk(vblk->disk);
>  out_free_tags:
>  	blk_mq_free_tag_set(&vblk->tag_set);
>  out_free_vq:
>  	vdev->config->del_vqs(vdev);
>  	kfree(vblk->vqs);
>  	vblk->ctrl_vq.vq = NULL;
>  out_free_vblk:
>  	kfree(vblk);

[Severity: High]
This is a pre-existing issue, but does this error cleanup path have a
use-after-free and double free?

The call to put_disk(vblk->disk) drops the reference count, invoking
virtblk_free_disk() which frees vblk. The subsequent labels continue to
dereference vblk->tag_set and vblk->vqs, and eventually kfree(vblk) is called
again.

[ ... ]

> @@ -1798,6 +2345,9 @@ static int virtblk_restore_priv(struct virtio_device *vdev)
>  	virtio_device_ready(vdev);
>  	blk_mq_unquiesce_queue(vblk->disk->queue);
>  
> +	if (vblk->profile.slots)
> +		blk_crypto_reprogram_all_keys(&vblk->profile);
> +
>  	return 0;
>  }

[Severity: High]
Could this expose unprovisioned keyslots during resume?

The hardware queues are unquiesced before blk_crypto_reprogram_all_keys() is
called. This instantly restarts I/O dispatch, meaning any pending encrypted
requests could be executed by the hypervisor with empty or invalid keyslots
before they are reprogrammed.

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