Re: [PATCH v18 4/7] firmware: arm_rmm: Add support for SRO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `delegated_phys` is used uninitialized on the buggy RMM error path of `rmi_delegate_range()`, causing unpredictable undelegation sizes and leaking/corrupting memory.
--

commit 860fe4975d9c1564e207c537f0159673fd509495
Author: Suzuki K Poulose <suzuki.poulose@arm.com>

firmware: arm_rmm: Add support for SRO

RMM v2.0 introduces Stateful RMI Operations (SRO) where an SMC can return with
an operation still in progress. The SRO is tracked using a struct rmi_sro_state
object to manage memory donation and reclamation. This commit provides the core
infrastructure for SRO and adds delegate/undelegate range wrappers that use it.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index 5b0e342ce3d58..4f9898ece7547 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c
[ ... ]
> +int rmi_delegate_range(phys_addr_t phys,
> +		       unsigned long size,
> +		       phys_addr_t *out_phys)
> +{
> +	long ret = 0;
> +	unsigned long top = phys + size;
> +	unsigned long out_top;
> +
> +	while (phys < top) {
> +		ret = rmi_granule_range_delegate(phys, top, &out_top);
> +
> +		if (ret == RMI_SUCCESS) {
> +			/* Buggy RMM ? */
> +			if (WARN_ON(out_top <= phys)) {
> +				rmi_undelegate_range(top - size, size);
> +				return -ENXIO;
> +			}

[Severity: High]
If we take this buggy RMM error path, this function returns -ENXIO immediately
without setting *out_phys. Does this leave the caller's delegated_phys variable
uninitialized?

Also, doesn't rmi_undelegate_range(top - size, size) here undo the entire
delegation, making the caller's own cleanup logic redundant and potentially
conflicting?

> +			phys = out_top;
> +		} else {
> +			break;
> +		}
> +	}
> +
> +	if (out_phys)
> +		*out_phys = phys;
> +
> +	return ret;
> +}
[ ... ]
> +static int rmi_sro_donate_contig(struct rmi_sro_state *sro,
> +				 unsigned long sro_handle,
> +				 unsigned long donatereq,
> +				 struct arm_smccc_1_2_regs *out_regs,
> +				 gfp_t gfp)
> +{
[ ... ]
> +	if (state == RMI_OP_MEM_DELEGATED) {
> +		phys_addr_t delegated_phys;
> +
> +		if (rmi_delegate_range(phys, size, &delegated_phys)) {
> +			if (!rmi_undelegate_range(phys, delegated_phys - phys))
> +				free_pages_exact(virt, size);
> +			return -ENXIO;
> +		}

[Severity: High]
If rmi_delegate_range() returns an error from the buggy RMM path,
delegated_phys is completely uninitialized.

Can this result in passing a garbage size (delegated_phys - phys) to
rmi_undelegate_range(), attempting to undelegate an unpredictable physical
memory region and preventing free_pages_exact() from executing?

> +	}
[ ... ]
> +static int rmi_sro_donate_noncontig(struct rmi_sro_state *sro,
> +				    unsigned long sro_handle,
> +				    unsigned long donatereq,
> +				    struct arm_smccc_1_2_regs *out_regs,
> +				    gfp_t gfp)
> +{
[ ... ]
> +		if (state == RMI_OP_MEM_DELEGATED) {
> +			phys_addr_t delegated_phys;
> +
> +			if (rmi_delegate_range(phys, block_size,
> +					       &delegated_phys)) {
> +				if (!rmi_undelegate_range(phys, delegated_phys - phys))
> +					free_pages_exact(virt, block_size);
> +				return -ENXIO;
> +			}

[Severity: High]
Similar to rmi_sro_donate_contig(), this code uses delegated_phys uninitialized
if rmi_delegate_range() takes the buggy RMM error path. Could this lead to
a similar memory leak and corruption risk?

> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912083611.2513845-1-suzuki.poulose@arm.com?part=4