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

[sashiko-bot@kernel.org]

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.

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

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

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

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