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. [ ... ] > 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/[email protected]?part=2
