On 9/14/2026 10:19 PM, [email protected] 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 <[email protected]>
>
> 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.
>
