Re: [PATCH v20 4/9] firmware: arm_rmm: Add support for SRO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] arm_rmm: SRO cancellation discards partial progress limit leading to memory corruption
- [Medium] arm_rmm: integer overflow in granule clamp bypasses RMM protection
- [Medium] arm_rmm: missing validation of block_size_fld leads to division by zero
--

commit 44837285c82689401d14180cb932077aa7982b67
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Add support for SRO

This commit introduces support for Stateful RMI Operations (SRO), allowing RMI
commands to return incomplete and be resumed. It adds SRO state tracking,
memory donation and reclaim handling, and wrappers for SRO commands.

> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c
> @@ -14,6 +15,687 @@
> +static long rmi_granule_range_delegate(unsigned long base,
> +				       unsigned long top,
> +				       unsigned long *out_top)
> +{
> +	struct arm_smccc_1_2_regs regs = {
> +		SMC_RMI_GRANULE_RANGE_DELEGATE, base, top
> +	};
> +	long ret = rmi_sro_execute(&regs);
> +
> +	if (ret == RMI_SUCCESS && out_top)
> +		*out_top = regs.a1;
> +
> +	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)
> +{
[ ... ]
> +mem_donate:
> +	rmi_op_mem_donate(sro_handle,
> +			  virt_to_phys(&sro->addr_list[sro->addr_count]), 1,
> +			  0, out_regs);
> +	donated_size = out_regs->a1 << PAGE_SHIFT;
> +
> +	if (WARN_ON(donated_size > size))
> +		donated_size = size;

[Severity: Medium]
Is there a risk of an integer overflow here before the bounds check?

If a buggy or compromised RMM returns a maliciously large value for
out_regs->a1, the left shift by PAGE_SHIFT could overflow the 64-bit
donated_size variable. This would result in a small value that bypasses
the WARN_ON() check.

The host kernel might then erroneously believe the RMM consumed fewer
granules than it actually did, freeing the remaining memory back to the
allocator while the RMM is actively using it. This could result in memory
corruption.

[ ... ]
> +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)
> +{
> +	unsigned long block_size_fld = RMI_DONATE_BLOCK_SIZE(donatereq);
> +	unsigned long block_size = rmi_addr_block_size_to_bytes(block_size_fld);
> +	unsigned long count = RMI_DONATE_COUNT(donatereq);
> +	unsigned long state = RMI_DONATE_STATE(donatereq);
> +	unsigned long found = 0;
> +	unsigned long donated_granules;
> +	unsigned long granules_per_block = block_size >> PAGE_SHIFT;
> +	unsigned long consumed_blocks;
[ ... ]
> +	donated_granules = out_regs->a1;
[ ... ]
> +	consumed_blocks = donated_granules / granules_per_block;

[Severity: Medium]
Could an unvalidated block_size_fld lead to a division by zero panic here?

While count and state are explicitly validated against anomalies,
block_size_fld from the firmware appears to be blindly trusted. If the
firmware returns an out-of-bounds value (e.g., 6), the shift amount in
rmi_addr_block_size_to_bytes() can evaluate modulo 64, yielding a block_size
smaller than PAGE_SIZE.

This would cause granules_per_block to evaluate to 0, triggering a kernel
panic when donated_granules is divided by granules_per_block.

[ ... ]
> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs)
> +{
> +	bool cancelled = false;
> +	unsigned long sro_handle;
[ ... ]
> +	if (cancelled)
> +		return -ECANCELED;
> +
> +	return regs->a0;
> +}

[Severity: Medium]
When an operation is cancelled, does this unconditionally discard the
output registers containing the partial progress limit?

If a buggy or compromised RMM returns an unexpected MEMREQ during a non-memory
SRO like SMC_RMI_GRANULE_RANGE_DELEGATE, rmi_sro_execute() issues an
RMI_OP_CANCEL and unconditionally returns -ECANCELED. This drops the final
state in the a1 register.

As a result, callers like rmi_granule_range_delegate() fail to capture
the progress limit. The kernel would then undelegate fewer granules
than the RMM successfully processed before returning them to the normal
allocator, which can lead to fatal Granule Protection Faults when the host
later accesses them.

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