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

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

On 9/20/2026 8:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] block: memory leak of sensitive cryptographic key material in virtio_blk
> - [High] block: DMA cacheline sharing in virtblk_ctrl_request causes data corruption
> - [High] block: deadlock in virtblk_crypto_generate_key() due to vdev_mutex
> - [Medium] block: incorrect bitwise arithmetic in get_supported_blk_key_types() breaks negotiation
> - [Medium] block: Kconfig VIRTIO_BLK_INLINE_ENCRYPTION tristate causes linker errors
> --
> 
> commit 3dd149ac489e952ca2319a45325b2d578747f639
> 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 the guest to offload per-I/O encryption to the host. It wires up
> device characteristics to a blk_crypto_profile and manages keys via the
> control virtqueue.
> 
>> 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: Medium]
> Should this option be a bool instead of a tristate?
> 
> If a user configures CONFIG_VIRTIO_BLK=y and CONFIG_BLK_INLINE_ENCRYPTION=m,
> this dependency evaluates to 'm'. Because the inline encryption code is
> integrated directly into virtio_blk.c using IS_ENABLED(), it gets compiled
> as built-in alongside virtio_blk. It will then attempt to link against
> blk_crypto_profile_init() and other block crypto symbols which are loadable
> modules, causing unresolved symbol errors during linking.

No. it should be 'tristate', so because it's code is totally in virtio block driver.
keeping 'tristate' make it possible for virtio block driver in module form can
support inline encryption as well - both CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION and
CONFIG_VIRTIO_BLK set as 'm'.

> 
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 2fad86e8f7a9a..30c303364ca97 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
> [ ... ]
>> @@ -124,7 +136,21 @@ 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]
> Could this struct layout cause cacheline corruption on architectures with
> non-coherent DMA?
> 
> The virtblk_ctrl_request struct dynamically allocates fields mapped for 
> DMA_TO_DEVICE (type, out_req) and DMA_FROM_DEVICE (status, in_resp),
> alongside CPU-modified fields (abandoned, compl). Because the struct is
> not cacheline aligned internally, these fields share CPU cachelines. If the
> CPU writes to the abandoned field while a DMA operation is occurring, or
> before dma_unmap_sg() invalidates the cache for DMA_FROM_DEVICE, the cache
> coherency conflict might overwrite or discard data.

ACK

Fix it by moving both struct completion and bool abandoned to a pointer.

> 
> [ ... ]
>> +static int virtblk_crypto_keyslot_program(struct blk_crypto_profile *profile,
>> +					   const struct blk_crypto_key *key,
>> +					   unsigned int slot)
>> +{
> [ ... ]
>> +	creq->type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_CRYPTO_KEYSLOT_PROGRAM);
>> +
>> +	err = set_virtblk_crypto_key_desc(vblk->vdev, creq, key, slot);
>> +	if (err)
>> +		goto out_free;
>> +
>> +	sg_init_one(&type_sg, &creq->type, sizeof(creq->type));
>> +	sg_init_one(&out_req_sg, &creq->out_req.key_desc, sizeof(creq->out_req.key_desc));
>> +	sg_init_one(&status_sg, &creq->status, sizeof(creq->status));
>> +	sgs[0] = &type_sg;
>> +	sgs[1] = &out_req_sg;
>> +	sgs[2] = &status_sg;
>> +
>> +	err = virtblk_ctrl_vq_request(vblk, creq, sgs, 2, 1);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does virtblk_ctrl_vq_request() contain a
> race condition when handling timeouts for these allocated control requests?
> 
> If wait_for_completion_timeout() times out, virtblk_ctrl_vq_request() sets
> creq->abandoned = true to inform the callback to free the memory. However, if
> the interrupt fires exactly after the timeout expires but before the lock is
> acquired, virtblk_ctrlq_callback() reads abandoned == false and assumes the
> waiting thread will free it. The waiting thread then returns -ETIMEDOUT and
> skips freeing it, leading to a permanent leak of both creq and compl.

ACK

Move struct completion and bool abandoned in struct virtblk_ctrl_request
to virtblk_ctrl_completion *. one time kmalloc_obj() of
virtblk_ctrl_completion * allow them not in the scatterlist.

> 
>> +	if (err == -ETIMEDOUT)
>> +		goto out_unlock;
>> +	if (err)
>> +		goto out_free;
>> +
>> +	err = blk_status_to_errno(virtblk_result(creq->status));
>> +out_free:
>> +	kfree(creq);
> 
> [Severity: High]
> Does this leak sensitive cryptographic key material in memory?
> 
> The creq buffer contains the plaintext or wrapped key descriptor, but it is
> freed via kfree() instead of kfree_sensitive(). The fscrypt subsystem rules
> mandate that keys must be zeroized after use to prevent memory disclosure.
> 
> [ ... ]
>> +static int virtblk_crypto_derive_sw_secret(struct blk_crypto_profile *profile,
>> +					    const u8 *eph_key, size_t eph_key_size,
>> +					    u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE])
>> +{
> [ ... ]
>> +	err = blk_status_to_errno(virtblk_result(creq->status));
>> +	if (err)
>> +		goto out_free;
>> +
>> +	memcpy(sw_secret, creq->in_resp.secret.secret, BLK_CRYPTO_SW_SECRET_SIZE);
>> +out_free:
>> +	kfree(creq);
> 
> [Severity: High]
> Similar to virtblk_crypto_keyslot_program(), does this leave the highly
> sensitive derived software secret in memory instead of zeroizing it with
> kfree_sensitive()?
> 
> [ ... ]
>> +static int virtblk_crypto_generate_key(struct blk_crypto_profile *profile,
>> +					u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE])
>> +{
>> +	struct virtio_blk *vblk = virtblk_from_profile(profile);
>> +	struct scatterlist type_sg, resp_sg, status_sg, *sgs[3];
>> +	struct virtblk_ctrl_request *creq;
>> +	unsigned int key_size;
>> +	int err;
>> +
>> +	mutex_lock(&vblk->vdev_mutex);
>> +	if (!vblk->vdev) {
>> +		err = -ENXIO;
>> +		goto out_unlock;
>> +	}
>> +
>> +	creq = kzalloc_obj(*creq, GFP_KERNEL);
> 
> [Severity: High]
> Can holding vdev_mutex during a GFP_KERNEL allocation cause a deadlock here?
> 
> Under memory pressure, GFP_KERNEL triggers direct reclaim, which flushes
> dirty pages to the block device. If the bio requires inline encryption, the
> block layer synchronously calls virtblk_crypto_keyslot_program() from the I/O
> submission path. That callback immediately attempts to acquire vdev_mutex,
> which is already held by the sleeping thread, causing a strict AB-BA/AA
> deadlock.
> 
> [ ... ]
>> +static unsigned int get_supported_blk_key_types(u8 virtio_key_types)
>> +{
>> +	unsigned int supported = 0;
>> +
>> +	if (virtio_key_types & VIRTIO_BLK_CRYPTO_KEY_TYPE_RAW)
>> +		supported |= virtio_key_type_to_blk(VIRTIO_BLK_CRYPTO_KEY_TYPE_RAW);
>> +	if (virtio_key_types & VIRTIO_BLK_CRYPTO_KEY_TYPE_HW_WRAPPED)
>> +		supported |= virtio_key_type_to_blk(VIRTIO_BLK_CRYPTO_KEY_TYPE_HW_WRAPPED);
>> +
>> +	return supported;
>> +}
> 
> [Severity: Medium]
> Does this construct an invalid capability bitmask by directly bitwise ORing
> raw enum values instead of shifting them?
> 
> The block layer's profile->key_types_supported strictly requires a bitmask
> where support is represented as (1 << BLK_CRYPTO_KEY_TYPE_...). For
> BLK_CRYPTO_KEY_TYPE_HW_WRAPPED (value 2), the accumulated enum value sets
> bit 1, making supported equal to 3. The core block layer later checks
> 3 & (1 << 2), which evaluates to 0, incorrectly causing
> blk_crypto_config_supported() to reject hardware-wrapped keys as unsupported.

No need shifting them here.

The return value of get_supported_blk_key_types is already a bitmask. Only
when virtio_key_types contains both VIRTIO_BLK_CRYPTO_KEY_TYPE_RAW and
VIRTIO_BLK_CRYPTO_KEY_TYPE_HW_WRAPPED, the 'supported' is 3, which means
both BLK_CRYPTO_KEY_TYPE_HW_WRAPPED key and BLK_CRYPTO_KEY_TYPE_RAW are
supported, which is expected.


>