Re: [PATCH v1 09/11] soc: qcom: add ICE keyslot partitioning driver for guest VMs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Probe error path leaves a dangling pointer in the global `g_ice_slots`, leading to a Use-After-Free.
- [Medium] Implicit assumptions about Device Tree node ordering falsely reject valid, disjoint keyslot allocations.
--

commit 298e1c705033929570b5634f841cfc46474229cd
Author: linlzhan <[email protected]>

soc: qcom: add ICE keyslot partitioning driver for guest VMs

On Qualcomm platforms the ICE hardware has a fixed number of physical
keyslots shared across the host and all guest VMs. Add the QCOM_ICE_SLOTS
platform driver to parse qcom,ice-keyslot-map DT nodes and map guest 
virtual keyslot indices to physical keyslots allocated to each VM.

> diff --git a/drivers/soc/qcom/qcom_ice_slots.c b/drivers/soc/qcom/qcom_ice_slots.c
> new file mode 100644
> index 0000000000000..364ac93077c12
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_ice_slots.c
[ ... ]
> +static int qcom_ice_slots_get_guest_slots(struct blk_crypto_profile *profile,
> +					  u32 guest_id)
> +{
> +	struct qcom_ice_slots *virt = virt_lookup(profile);
> +	unsigned int i;
> +
> +	if (!virt)
> +		return -ENOKEY;
> +
> +	/* entries[0] is the host; guest entries start at index 1. */
> +	for (i = 1; i < virt->num_entries; i++) {

[Severity: Medium]
Does this loop skip index 0 by hardcoding the assumption that the very first
parsed DT child node corresponds to the host?

This hides the first parsed entry from guest translation regardless of its
actual properties, which breaks guest VM keyslot partitioning if the host is
not the first child node in the device tree.

> +		if (virt->entries[i].guest_id == guest_id)
> +			return virt->entries[i].max_slots;
> +	}
> +	return -ENOKEY;
> +}
[ ... ]
> +static int qcom_ice_slots_probe(struct platform_device *pdev)
> +{
[ ... ]
> +		if (idx > 0 &&
> +		    slot_offset <
> +		    virt->entries[idx - 1].slot_offset +
> +		    virt->entries[idx - 1].max_slots) {

[Severity: Medium]
Can this logic falsely abort the probe when the nodes are disjoint but appear
out-of-order in the Device Tree?

This check only verifies the current entry's offset against the immediate
predecessor's end. If the nodes are legitimately sorted by their unit-address
reg instead of slot_offset, this mathematically flawed check evaluates to true
and falsely rejects valid physical keyslot mappings.

> +			dev_err(dev, "slot overlap: guest_id=%u overlaps guest_id=%u\n",
> +				guest_id, virt->entries[idx - 1].guest_id);
> +			ret = -EINVAL;
> +			of_node_put(child);
> +			goto err_free;
> +		}
[ ... ]
> +	virt->num_entries = idx;
> +
> +	/*
> +	 * Publish the singleton.  From this point on, bcp_slot_virt_ops
> +	 * callbacks can resolve virt via rcu_dereference(g_ice_slots).
> +	 */
> +	rcu_assign_pointer(g_ice_slots, virt);
> +
> +	ret = bcp_register_slot_virt_ops(&qcom_slot_virt_ops);
> +	if (ret) {
> +		dev_err(dev, "failed to register slot_virt_ops: %d\n", ret);
> +		goto err_free;
> +	}

[Severity: High]
Is it possible for this error path to leave a dangling pointer in the global
g_ice_slots resulting in a use-after-free regression?

The devm-allocated virt state is published to the global g_ice_slots pointer
via rcu_assign_pointer(). If bcp_register_slot_virt_ops() fails, the function
returns via the err_free label without setting g_ice_slots back to NULL. 

When the devres framework subsequently frees virt, it leaves a dangling pointer
that can be dereferenced by the hot path if blk-crypto-proxy invokes the
still-registered operations.

> +
> +	dev_info(dev, "registered: %u VMs, %u total ICE slots\n",
> +		 idx, total_slots);
> +	return 0;
> +
> +err_free:
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9