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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
[ ... ]
quoted hunk ↗ jump to hunk
+static int virtblk_crypto_keyslot_program(struct blk_crypto_profile *profile,
+ const struct blk_crypto_key *key,
+ unsigned int slot)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
[ ... ]
quoted hunk ↗ jump to hunk
+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])
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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()?
[ ... ]
quoted hunk ↗ jump to hunk
+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.
[ ... ]
quoted hunk ↗ jump to hunk
+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