Re: [PATCH v1 11/11] scsi: ufs: ufs-qcom: support ICE keyslot partitioning for guest VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer overflow in ICE keyslot boundary checks allows out-of-bounds hardware register access.
- [High] Missing overlap validation for keyslot partitions causes silent data corruption across VMs.
- [High] Inability to distinguish between a missing DT node and a 0-slot host reservation overrides explicit 0-slot allocations, breaking VM partitioning.
--

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

scsi: ufs: ufs-qcom: support ICE keyslot partitioning for guest VMs

This commit adds support for partitioning ICE hardware keyslots so that the
host and guest VMs can operate within their own physical slot ranges. It
introduces a new function to parse the keyslot map from the device tree and
initializes the crypto profile with the host's specific slot count.

> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 62396212a0a73..0611ab50f4cc9 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
[ ... ]
> +static int ufs_qcom_ice_parse_slot_table(struct device *dev,
> +					 unsigned int hw_max_slots,
> +					 unsigned int *num_slots,
> +					 unsigned int *slot_offset)
> +{
> +	struct device_node *slots_np, *child;
> +	unsigned int total_slots = 0;
> +	bool first = true;
> +	int ret = 0;
> +
> +	*num_slots   = 0;
> +	*slot_offset = 0;
> +
> +	slots_np = of_parse_phandle(dev->of_node, "qcom,ice-keyslot-map", 0);
> +	if (!slots_np)
> +		return 0;
> +
> +	for_each_child_of_node(slots_np, child) {
> +		u32 off, max;
> +
> +		if (of_property_read_u32(child, "qcom,ice-slot-offset", &off) ||
> +		    of_property_read_u32(child, "qcom,max-ice-slots", &max))
> +			continue;
> +
> +		if (off + max > hw_max_slots) {

[Severity: High]
If a malformed device tree provides excessively large values for
qcom,ice-slot-offset and qcom,max-ice-slots, can off + max wrap around
the 32-bit limit? This could defeat the bounds check and potentially
allow out-of-bounds hardware register accesses.

> +			dev_err(dev,
> +				"ice-keyslot-map: slots [%u..%u) exceed hw max %u\n",
> +				off, off + max, hw_max_slots);
> +			of_node_put(child);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (first) {
> +			*num_slots   = max;
> +			*slot_offset = off;
> +			first = false;
> +		}
> +		total_slots += max;

[Severity: High]
Can total_slots overflow a 32-bit integer when adding max? This could
defeat the subsequent check that ensures the total allocated slots do not
exceed the hardware maximum.

[Severity: High]
Does this logic only sum the capacities and miss bounds collision detection
between different VM partition ranges?

If the explicit slot intervals overlap in the device tree configuration,
could the host and guests silently overwrite each other's keys because the
total sum still fits within the hardware maximum?

> +	}
> +
> +	of_node_put(slots_np);
[ ... ]
> @@ -192,7 +262,23 @@ static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
[ ... ]
> +	/*
> +	 * Parse the qcom,ice-keyslot-map DT node: validate all entries against
> +	 * the hardware slot count and read the host's own reservation.  If no
> +	 * partitioning is configured (num_slots == 0), the profile manages the
> +	 * full hardware slot range.
> +	 */
> +	err = ufs_qcom_ice_parse_slot_table(dev, hw_max_slots,
> +					    &num_slots, &slot_offset);
> +	if (err) {
> +		dev_err(dev, "failed to parse ICE slot table: %d\n", err);
> +		return err;
> +	}
> +
> +	err = devm_blk_crypto_profile_init(dev, profile,
> +					   num_slots ? num_slots : hw_max_slots);

[Severity: High]
Since ufs_qcom_ice_parse_slot_table() initializes num_slots to 0 and returns
0 when the device tree node is absent, is it possible to distinguish between
a missing node and a valid 0-slot host allocation?

If a device tree explicitly allocates 0 slots to the host to dedicate all
physical keyslots to VMs, does this ternary operation conflate the valid
empty partition with a missing configuration and forcefully assign all
hardware slots to the host's crypto profile?

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