Re: [PATCH v1 08/11] block: add /dev/blk-crypto-proxy for host-side virtio-blk inline encryption

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Kernel crash when calling import_iovec with iov_cnt == 1 and a NULL iov pointer.
- [High] Sleeping in Atomic Context inside bcp_ioctl_bind_context.
- [High] UAPI mismatch causes silent data corruption for 32-bit compat userspace processes.
- [High] Pinned user pages are leaked unconditionally on every successful or failed bio submission.
- [High] The driver passes guest-provided data_unit_size_bits directly to hardware without validating it against the block device's supported crypto profile.
- [High] Missing little-endian to host-endian conversion for arg.dun.
- [Low] Dead error handling path for bio_alloc() using GFP_KERNEL.
--

commit 654e63cd96fb66a7cdbe2a3c22748ad74f166ab2
Author: linlzhan <linlin.zhang@oss.qualcomm.com>

block: add /dev/blk-crypto-proxy for host-side virtio-blk inline encryption
    
This patch introduces a misc character device to allow userspace virtio-blk
backends to submit inline-encrypted block I/O on behalf of virtual machines.
It maps virtual keyslots to physical hardware keyslots and submits bios
synchronously.

> diff --git a/drivers/block/blk-crypto-proxy.c b/drivers/block/blk-crypto-proxy.c
> index 0000000000000..60722884dbcd7
> --- /dev/null
> +++ b/drivers/block/blk-crypto-proxy.c
[ ... ]
> @@ -180,11 +180,11 @@ static long bcp_ioctl_bind_context(struct file *file,
> +	/*
> +	 * get_guest_id() may sleep; call it before taking bind_lock.
> +	 */
> +	rcu_read_lock();
> +	hv_ops = rcu_dereference(g_hypervisor_ops);
> +	if (!hv_ops) {
> +		rcu_read_unlock();
> +		return -EOPNOTSUPP;
> +	}
> +	ret = hv_ops->get_guest_id(arg.vm_fd, &guest_id);

[Severity: High]
Since the comment explicitly mentions that get_guest_id() may sleep, is it
safe to call it while holding the RCU read lock? Sleeping in an RCU
read-side critical section typically leads to scheduling while atomic
warnings or panics.

> +	rcu_read_unlock();
[ ... ]
> @@ -411,10 +411,10 @@ static long bcp_ioctl_submit_io_by_vslot(struct file *file,
[ ... ]
> +	memset(dun, 0, sizeof(dun));
> +	dun[0] = arg.dun;

[Severity: High]
Does this assignment need a little-endian to host-endian conversion? The
UAPI specifies arg.dun as little-endian, but it is being assigned verbatim
to dun[0] which the crypto subsystem expects to be host-endian. On a
big-endian system, this might result in an incorrect data unit number and
silent decryption failures.

> +
> +	slot.phy_slot            = phy_slot;
> +	slot.data_unit_size_bits = arg.data_unit_size_bits;

[Severity: High]
Is it safe to pass the guest-provided data_unit_size_bits directly into the
crypto slot configuration without validating it against the block device's
supported crypto profile? For slot-based bios, the block layer skips
blk_crypto_config_supported_natively(), meaning an invalid unit size could
be pushed directly to the hardware driver.

> +
> +	align = 1ULL << arg.data_unit_size_bits;
[ ... ]
> +	/*
> +	 * Import the caller's iovec once.  import_iovec() validates every
> +	 * segment with access_ok(), returns the total byte count, and takes a
> +	 * private kernel copy that eliminates TOCTOU from a guest mutating its
> +	 * own iovec array mid-ioctl.
> +	 */
> +	ret = import_iovec(arg.direction == BCP_DIR_READ ? ITER_DEST : ITER_SOURCE,
> +			   (const struct iovec __user *)u64_to_user_ptr(arg.iov_ptr),
> +			   arg.iov_cnt, 0, &iov, &iter);

[Severity: Critical]
Can this crash if a userspace process provides iov_cnt == 1 and a NULL
iov_ptr? The fast_segs parameter is set to 0 and iov is initialized
to NULL. When nr_segs is 1, import_iovec() will clear the iov pointer and
pass it to copy_iovec_from_user(), which directly writes to the NULL pointer.

[Severity: High]
Does this also cause an issue for 32-bit compat userspace processes? The
UAPI bcp_iovec is exactly 16 bytes. However, when in_compat_syscall() is
true, import_iovec() parses the input using copy_compat_iovec_from_user(),
which assumes 8-byte array elements (struct compat_iovec). This UAPI
mismatch could lead to memory corruption or misdirected block I/O.

> +	if (ret < 0)
> +		return ret;
[ ... ]
> +	/*
> +	 * Reject an out-of-range DUN: slot-based bios skip
> +	 * bio_crypt_check_alignment(), so an overflow would silently truncate
> +	 * in the hardware DUN field rather than error out.
> +	 */
> +	{
> +		u64 total_units = total_bytes >> arg.data_unit_size_bits;
> +		u64 max_dun_used, dun_limit;
> +
> +		if (check_add_overflow(arg.dun, total_units - 1, &max_dun_used)) {

[Severity: High]
Could this overflow check yield incorrect results on big-endian hosts? It
performs host-endian arithmetic directly on arg.dun, which is a little-endian
value according to the UAPI.

> +			ret = -EINVAL;
> +			goto out;
> +		}
[ ... ]
> +		if (bio_bytes == 0)
> +			break;
> +
> +		bio = bio_alloc(bdev, pages_used,
> +				arg.direction == BCP_DIR_WRITE ?
> +					REQ_OP_WRITE : REQ_OP_READ,
> +				GFP_KERNEL);
> +		if (!bio) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}

[Severity: Low]
Can this allocation ever fail? bio_alloc() is backed by a mempool and is
called here with GFP_KERNEL (which implies __GFP_DIRECT_RECLAIM). Standard
block subsystem guidelines suggest this failure path is unreachable. Treating
it as reachable can lead to incorrect assumptions about memory allocation
behaviors.

> +		bio->bi_iter.bi_sector = arg.sector + (bytes_done >> SECTOR_SHIFT);
[ ... ]
> +		remaining_before = iov_iter_count(&iter);
> +		iov_iter_truncate(&iter, bio_bytes);
> +		ret = bio_iov_iter_get_pages(bio, &iter, 0, 0);
> +		if (ret < 0) {
> +			bio_put(bio);
> +			goto out;
> +		}
> +		if (iov_iter_count(&iter) != 0) {
> +			/*
> +			 * The lookahead verified bio_bytes fits in BIO_MAX_VECS;
> +			 * if bio_iov_iter_get_pages() stopped early, its page
> +			 * accounting disagreed with bcp_iov_pages_for_bytes().
> +			 */
> +			bio_put(bio);
> +			ret = -EIO;
> +			goto out;
> +		}

[Severity: High]
When iov_iter_count is not zero, bio_put() is called, but it does not
implicitly release the pages pinned by bio_iov_iter_get_pages(). Should this
error path explicitly call bio_release_pages(bio, false) to prevent memory
leaks?

> +		iov_iter_reexpand(&iter, remaining_before - bio_bytes);
[ ... ]
> +		bio_crypt_set_ctx_by_slot(bio, &slot, dun, GFP_KERNEL);
> +
> +		ret = submit_bio_wait(bio);
> +		bio_put(bio);
> +		if (ret)
> +			goto out;

[Severity: High]
Similar to the error path above, should there be a call to
bio_release_pages(bio, false) before or after bio_put() finishes? Otherwise
the pinned user pages appear to be leaked unconditionally for every bio
submitted.

> +
> +		/*

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