Re: [PATCH v1 01/11] virtio_blk: add inline encryption support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The singleton inline crypto profile stores a pointer to the first probed virtio-blk device, leading to a Use-After-Free when that device is unbound.
- [Medium] Missing validation of crypto_modes_supported when reusing the singleton blk_crypto_profile across multiple devices.
- [Low] Accessing `req->crypt_ctx` and `req->crypt_keyslot` without `#ifdef CONFIG_BLK_INLINE_ENCRYPTION` breaks compilation when inline encryption is disabled.
- [Low] A reversed Kconfig dependency between VIRTIO_BLK and VIRTBLK_CRYPTO_VIRTUALIZATION causes a vmlinux linkage failure when VIRTIO_BLK is built-in and the extension is a module.
--

commit a4edc1b4437c893d0130908c0c7ae81f9e6802b0
Author: linlzhan <linlzhan@qti.qualcomm.com>

virtio_blk: add inline encryption support

Negotiate VIRTIO_BLK_F_INLINE_ENCRYPTION with the host and wire it into
the block layer's inline-crypto framework to enable inline encryption
on virtio block device.

> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 858320b6ebb7e..7790ee2c700c4 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -312,6 +312,19 @@ config VIRTIO_BLK
>  	  This is the virtual block driver for virtio.  It can be used with
>            QEMU based VMMs (like KVM or Xen).  Say Y or M.
>  
> +config VIRTBLK_CRYPTO_VIRTUALIZATION
> +	tristate "Virtio block inline encryption virtualization support"
> +	depends on VIRTIO_BLK && BLK_INLINE_ENCRYPTION

[Severity: Low]
Can this combination cause a linker error? If VIRTIO_BLK=y and
VIRTBLK_CRYPTO_VIRTUALIZATION=m, IS_ENABLED(CONFIG_VIRTBLK_CRYPTO_VIRTUALIZATION)
evaluates to true in the built-in virtio_blk.o, causing it to call
virtblk_init_inline_crypto(). Since that function is in a separate module,
would this result in an undefined reference during the vmlinux link?

> +	help
> +	  Say 'Y or M' to enable routing of crypto requests to a different
> +	  operating system in a virtualized environment. This option by
> +	  itself does not provide a working backend: enable a
> +	  platform-specific driver that implements struct
> +	  virtblk_crypto_variant_ops as well (e.g. QCOM_CRYPTO_VIRT on
> +	  Qualcomm platforms). Without one, VIRTIO_BLK_F_INLINE_ENCRYPTION is
> +	  still negotiated and advertised to the block layer, but every
> +	  inline-crypto operation fails with -EOPNOTSUPP at runtime.
> +
>  config BLK_DEV_RBD
>  	tristate "Rados block device (RBD)"
>  	depends on INET && BLOCK
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 32bf3ba07a9dc..61a3967bb4dfe 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c

[ ... ]

> @@ -235,6 +249,15 @@ static void virtblk_cleanup_cmd(struct request *req)
>  		kfree(bvec_virt(&req->special_vec));
>  }
>  
> +static bool is_crypto_request(struct virtio_device *vdev, struct request *req)
> +{
> +	if (!IS_ENABLED(CONFIG_VIRTBLK_CRYPTO_VIRTUALIZATION) ||
> +	    !virtio_has_feature(vdev, VIRTIO_BLK_F_INLINE_ENCRYPTION))
> +		return false;
> +
> +	return req->crypt_ctx && req->crypt_keyslot;

[Severity: Low]
Does this cause a build failure when CONFIG_BLK_INLINE_ENCRYPTION is disabled?
Even though is_crypto_request() returns false, the compiler still evaluates
req->crypt_ctx and req->crypt_keyslot. Since these fields are conditionally
compiled out of struct request without inline encryption, accessing them here
and in virtblk_setup_cmd() might break the build.

> +}
> +
>  static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  				      struct request *req,
>  				      struct virtblk_req *vbr)

[ ... ]

> @@ -309,8 +339,8 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  
>  	/* Set fields for non-REQ_OP_DRV_IN request types */
>  	vbr->in_hdr_len = in_hdr_len;
> -	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
> -	vbr->out_hdr.sector = cpu_to_virtio64(vdev, sector);
> +	vbr->out_hdr.base.type = cpu_to_virtio32(vdev, type);
> +	vbr->out_hdr.base.sector = cpu_to_virtio64(vdev, sector);
>  
>  	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
>  	    type == VIRTIO_BLK_T_SECURE_ERASE) {
> @@ -318,6 +348,17 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
>  			return BLK_STS_RESOURCE;
>  	}
>  
> +	if (type == VIRTIO_BLK_T_CRYPTO_IN || type == VIRTIO_BLK_T_CRYPTO_OUT) {
> +		unsigned int slot = blk_crypto_keyslot_index(req->crypt_keyslot);
> +		unsigned int data_unit_size_bits = req->crypt_ctx->bc_key->data_unit_size_bits;
> +		u64 dun = req->crypt_ctx->bc_dun[0];

[Severity: Low]
Does this also cause a build failure when CONFIG_BLK_INLINE_ENCRYPTION is
disabled, since these conditionally compiled struct fields are directly
accessed without preprocessor guards?

> +
> +		vbr->out_hdr.crypto_append.msg.slot = cpu_to_virtio32(vdev, slot);
> +		vbr->out_hdr.crypto_append.msg.data_unit_size_bits =
> +				cpu_to_virtio32(vdev, data_unit_size_bits);
> +		vbr->out_hdr.crypto_append.msg.dun = cpu_to_virtio64(vdev, dun);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/block/virtio_blk_crypto_ext.c b/drivers/block/virtio_blk_crypto_ext.c
> new file mode 100644
> index 0000000000000..00b6d410d3036
> --- /dev/null
> +++ b/drivers/block/virtio_blk_crypto_ext.c

[ ... ]

> +int virtblk_init_inline_crypto(unsigned int max_slots, unsigned int max_dun_bytes,
> +			       unsigned int key_types,
> +			       const unsigned int crypto_modes_supported[BLK_ENCRYPTION_MODE_MAX],
> +			       struct device *dev)
> +{
> +	struct blk_crypto_profile *profile = &g_vdcp.profile;
> +	unsigned int key_type_supported = 0;
> +	int err = 0;
> +
> +	dev_info(dev, "probing inline crypto capabilities\n");
> +
> +	mutex_lock(&virtblk_crypto_init_lock);
> +
> +	/*
> +	 * profile is a single, process-wide blk_crypto_profile shared by every
> +	 * VIRTIO_BLK_F_INLINE_ENCRYPTION device. Only the first device to get
> +	 * here actually initializes it; any other device just reuses it as-is
> +	 * if its negotiated capabilities match. A mismatch means this device's
> +	 * capabilities don't actually correspond to what the shared profile
> +	 * was set up for (wrong keyslot count, DUN size, or key types), which
> +	 * is a correctness/security concern, not just a cosmetic one -- fail
> +	 * instead of silently registering a profile that doesn't match what
> +	 * this device supports.
> +	 */
> +	if (g_crypto_profile_initialized) {
> +		if (max_slots != g_max_slots || max_dun_bytes != g_max_dun_bytes ||
> +		    key_types != g_key_types) {

[Severity: Medium]
Does this logic skip validating the negotiated crypto_modes_supported array?
If a second device supports different cryptographic modes than the first one,
could it silently inherit the first device's modes, potentially causing the
block layer to send unsupported crypto requests?

> +			dev_warn(dev,
> +				 "inline crypto profile already initialized by %s (max_slots=%u max_dun_bytes=%u key_types=0x%x); "
> +				 "this device reports max_slots=%u max_dun_bytes=%u key_types=0x%x -- sharing one "
> +				 "blk_crypto_profile across multiple VIRTIO_BLK_F_INLINE_ENCRYPTION devices with differing "
> +				 "capabilities is not supported, refusing to enable inline crypto for this device\n",
> +				 dev_name(virtblk_profile_owner), g_max_slots, g_max_dun_bytes, g_key_types,
> +				 max_slots, max_dun_bytes, key_types);
> +			err = -EINVAL;
> +		}
> +		goto out_unlock;
> +	}
> +
> +	if (key_types & VIRTIO_BLK_CRYPTO_KEY_TYPE_RAW)
> +		key_type_supported |= BLK_CRYPTO_KEY_TYPE_RAW;
> +	if (key_types & VIRTIO_BLK_CRYPTO_KEY_TYPE_HW_WRAPPED)
> +		key_type_supported |= BLK_CRYPTO_KEY_TYPE_HW_WRAPPED;
> +
> +	err = blk_crypto_profile_init(profile, max_slots);
> +	if (err) {
> +		dev_err(dev, "crypto profile initialization failed: %d\n", err);
> +		goto out_unlock;
> +	}
> +
> +	profile->ll_ops = virtblk_crypto_ops;
> +	profile->max_dun_bytes_supported = max_dun_bytes;
> +	profile->key_types_supported = key_type_supported;
> +	profile->dev = dev;
> +	memcpy(profile->modes_supported, crypto_modes_supported,
> +	       BLK_ENCRYPTION_MODE_MAX * sizeof(unsigned int));
> +
> +	virtblk_profile_owner = dev;

[Severity: High]
Can this lead to a use-after-free?

profile->dev stores a pointer to the first probed virtio-blk device in a
globally shared singleton. If this device is hot-unplugged or unbound,
its struct device is freed.

When a subsequent device performs a crypto operation, could blk_crypto_hw_enter()
dereference this freed pointer when calling pm_runtime_get_sync(profile->dev)?

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