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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Infinite loop during SRO cancellation if the RMM continues to return an unexpected MEMREQ.
- [High] Missing bounds check on `out_regs->a1` in `rmi_sro_donate_contig` allows an integer overflow that can erroneously free actively consumed memory.
- [High] The error handler for `sro->addr_count` corruption in `rmi_sro_ensure_capacity` triggers an out-of-bounds read and arbitrary free.
- [High] Potential infinite loop in `rmi_delegate_range` and `rmi_undelegate_range` on buggy RMM.
--

commit c980046a949a7ff8e2698f0f01373297576d9896
Author: Steven Price <steven.price@arm.com>
Subject: firmware: arm_rmm: Add support for SRO

This commit introduces support for RMM v2.0 Stateful RMI Operations (SRO), allowing SMCs to return with operations still in progress. It adds struct rmi_sro_state to track memory allocated but not yet consumed or reclaimed, and provides wrappers for SRO RMI commands.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index 76f91c145e1fd..42c973c3a98bb 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c
> @@ -24,6 +25,513 @@ unsigned long rmi_feat_reg(unsigned long id)
>  }
>  EXPORT_SYMBOL_GPL(rmi_feat_reg);
>  
> +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)
> +			phys = out_top;

[Severity: High]
Could this loop indefinitely if a buggy RMM firmware returns RMI_SUCCESS but
does not advance out_top beyond phys?

> +		else if (ret == RMI_BUSY || ret == RMI_BLOCKED)
> +			cpu_relax();
> +		else
> +			break;
> +	}
> +
> +	if (out_phys)
> +		*out_phys = phys;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rmi_delegate_range);
> +
> +int rmi_undelegate_range(phys_addr_t phys,
> +			 unsigned long size)
> +{
> +	long ret = 0;
> +	unsigned long top = phys + size;
> +	unsigned long out_top;
> +
> +	while (phys < top) {
> +		ret = rmi_granule_range_undelegate(phys, top, &out_top);
> +		if (ret == RMI_SUCCESS)
> +			phys = out_top;

[Severity: High]
Does this undelegate loop carry the same infinite loop risk as the delegate path
if the hypervisor fails to advance out_top?

> +		else if (ret == RMI_BUSY || ret == RMI_BLOCKED)
> +			cpu_relax();
> +		else
> +			break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rmi_undelegate_range);

[ ... ]

> +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)
> +{

[ ... ]

> +	rmi_op_mem_donate(sro_handle,
> +			  virt_to_phys(&sro->addr_list[sro->addr_count]), 1,
> +			  0, out_regs);
> +
> +	unsigned long donated_granules = out_regs->a1;
> +	unsigned long donated_size = donated_granules << PAGE_SHIFT;

[Severity: High]
Can donated_granules (out_regs->a1) be an untrusted large value from a
compromised or buggy RMM?

If out_regs->a1 is excessively large, donated_granules << PAGE_SHIFT could
overflow into a small integer value.

> +
> +	if (donated_granules == 0) {
> +		/* No pages used by the RMM */
> +		sro->addr_count++;
> +	} else if (donated_size < size) {
> +		phys = sro->addr_list[sro->addr_count] & RMI_ADDR_RANGE_ADDR_MASK;
> +
> +		/* Not all granules used by the RMM, free the remaining pages */
> +		for (long i = donated_size; i < size; i += PAGE_SIZE) {

[Severity: High]
If the integer overflow described above occurs, this check
(donated_size < size) will incorrectly pass. Would this lead to the host
allocator erroneously freeing memory that the RMM might still be actively
using?

> +			if (state == RMI_OP_MEM_DELEGATED)
> +				free_delegated_page(phys + i);
> +			else
> +				__free_page(phys_to_page(phys + i));
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int rmi_sro_reclaim(struct rmi_sro_state *sro,
> +			   unsigned long sro_handle,
> +			   struct arm_smccc_1_2_regs *out_regs)
> +{
> +	unsigned long capacity;
> +	int ret;
> +
> +	ret = rmi_sro_ensure_capacity(sro, 1);
> +	if (ret)
> +		rmi_sro_free(sro);

[Severity: High]
Is it safe to pass sro to rmi_sro_free() here when rmi_sro_ensure_capacity()
failed?

If rmi_sro_ensure_capacity() detects corruption and returns -EOVERFLOW because
sro->addr_count > RMI_MAX_ADDR_LIST, calling rmi_sro_free() passes that same
corrupted addr_count to the cleanup loop.

> +
> +	capacity = RMI_MAX_ADDR_LIST - sro->addr_count;
> +
> +	rmi_op_mem_reclaim(sro_handle,
> +			   virt_to_phys(&sro->addr_list[sro->addr_count]),
> +			   capacity, out_regs);
> +
> +	if (WARN_ON_ONCE(out_regs->a1 > capacity))
> +		out_regs->a1 = capacity;
> +
> +	sro->addr_count += out_regs->a1;
> +
> +	return 0;
> +}
> +
> +void rmi_sro_free(struct rmi_sro_state *sro)
> +{
> +	for (int i = 0; i < sro->addr_count; i++) {

[Severity: High]
If sro->addr_count is corrupted and greater than RMI_MAX_ADDR_LIST, as caught
in the error path above, won't this loop use it as an upper bound, leading to
out-of-bounds array reads and arbitrary memory frees?

> +		unsigned long entry = sro->addr_list[i];
> +		unsigned long addr = RMI_ADDR_RANGE_ADDR(entry);
> +		unsigned long unit_size = RMI_ADDR_RANGE_SIZE(entry);
> +		unsigned long count = RMI_ADDR_RANGE_COUNT(entry);
> +		unsigned long state = RMI_ADDR_RANGE_STATE(entry);
> +		unsigned long size = donate_req_to_size(unit_size) * count;
> +
> +		if (state == RMI_OP_MEM_DELEGATED) {
> +			if (WARN_ON_ONCE(rmi_undelegate_range(addr, size))) {
> +				/* Leak the pages */
> +				continue;
> +			}
> +		}
> +		free_pages_exact(phys_to_virt(addr), size);
> +	}
> +
> +	sro->addr_count = 0;
> +}
> +EXPORT_SYMBOL_GPL(rmi_sro_free);

[ ... ]

> +long rmi_sro_memxfer_execute(struct rmi_sro_state *sro, gfp_t gfp)
> +{
> +	unsigned long sro_handle;
> +	struct arm_smccc_1_2_regs *regs = &sro->regs;
> +	bool cancelled = false;
> +
> +	rmi_smccc_invoke(regs, regs);
> +
> +	sro_handle = regs->a1;
> +
> +	while (RMI_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
> +		bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
> +		int ret = 0;
> +
> +		switch (RMI_RETURN_MEMREQ(regs->a0)) {

[ ... ]

> +		}
> +
> +		if (ret) {
> +			/*
> +			 * All memory donating SROs must be cancellable. So a
> +			 * failure in memory allocation shouldn't be an issue.
> +			 * However, if we encounter a random failure (e.g.,
> +			 * buggy RMM), don't loop forever, just give up.
> +			 */
> +			if (WARN_ON_ONCE(!can_cancel))
> +				return ret;
> +
> +			rmi_op_cancel(sro_handle, regs);
> +			cancelled = true;
> +
> +			if (WARN_ON_ONCE(RMI_RETURN_STATUS(regs->a0) != RMI_INCOMPLETE))
> +				return ret;

[Severity: High]
Does this sequence create an infinite loop on a buggy RMM?

If the RMM returns an unexpected MEMREQ and does not clear the can_cancel bit in
its return status after a cancellation is requested, the loop unconditionally
re-issues the cancel command because it does not check the local cancelled state.

> +		}
> +	}
> +
> +	if (cancelled)
> +		return -ECANCELED;
> +
> +	return regs->a0;
> +}
> +EXPORT_SYMBOL_GPL(rmi_sro_memxfer_execute);
> +
> +/* For RMI commands that are stateful but not memory-transferring */
> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs)
> +{
> +	unsigned long sro_handle;
> +	bool cancelled = false;
> +
> +	rmi_smccc_invoke(regs, regs);
> +
> +	sro_handle = regs->a1;
> +
> +	while (RMI_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
> +		bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0);
> +
> +		switch (RMI_RETURN_MEMREQ(regs->a0)) {
> +		case RMI_OP_MEM_REQ_NONE:
> +			rmi_op_continue(sro_handle, RMI_CONTINUE_KEEP_GOING,
> +					regs);
> +			break;
> +		default:
> +			WARN_ON_ONCE(1);
> +			if (!can_cancel)
> +				return regs->a0;
> +
> +			cancelled = true;
> +			rmi_op_cancel(sro_handle, regs);

[Severity: High]
Could the same infinite loop vulnerability occur here for non-memory-transferring
operations if the RMM continuously returns RMI_INCOMPLETE and an unexpected MEMREQ
without clearing can_cancel?

> +		}
> +	}
> +
> +	if (cancelled)
> +		return -ECANCELED;
> +
> +	return regs->a0;
> +}
> +EXPORT_SYMBOL_GPL(rmi_sro_execute);

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