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

[Linlin Zhang <linlin.zhang@oss.qualcomm.com>]

On 9/14/2026 10:19 PM, sashiko-bot@kernel.org wrote:
> 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.
> 

Setting it as 'tristate' here to ensure that VIRTIO_BLK with inline encryption
supoort can still be built as a module. I.e. if CONFIG_VIRTIO_BLK=m,
CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION must also be m.

> [ ... ]
> 
>> 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().
> 

ACK

update struct completion compl to a pointer of struct 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...
> 

This isn't a bug.

Because inline encryption is only added for the non-zoned device. REQ_OP_ZONE_FINISH
and REQ_OP_ZONE_APPEND are for zoned device, which doesn't have blk-crypto-profile
registered into its request queue.

> [ ... ]
> 
>> @@ -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.
> 

This is not a bug. It's excepted that the out header of virtio block request isn't
appened with crypto metadata for a zoned device.

> [ ... ]
> 
>> @@ -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.
> 

ACK

Transmit the kernel internal blk_crypto_mode_num to the virtio Spec defined
crypto mode num.

> [ ... ]
> 
>> +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.
> 

ACK

Replace GFP_KERNEL with GFP_NOIO

> [ ... ]
> 
>> +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.
> 

ACK

Replace GFP_KERNEL with GFP_NOIO

>> +
>> +	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.
> 

ACK

Protect key program/evict/derive_sw_secret/generate/prepare/import by acquiring
vdev_mutex lock.

> [ ... ]
> 
>> +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.
> 

ACK

Move the check of inline encryption support to a independent conditional branch.
Print the log in Zoned device case to show the mutual exclusion of zone device
and inline encryption support.

> [ ... ]
> 
>>  	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.
> 
> [ ... ]

This isn't a issue.

As GD_ADDED only be set in the successful case of device_add_disk(), and
disk->fops->free_disk() to virtblk_free_disk() is only called when GD_ADDED is set.

when device_add_disk() returns non-zero,  virtblk_free_disk() won't be called,
vblk isn't freed before vblk->ctrl_vq.vq = NULL.

> 
>> @@ -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.
>