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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi,

Responses inline.

On 29/09/2026 23:30, sashiko-bot@kernel.org wrote:
> 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.

Fair point, this was due to the removal of donated_granules. I will
rectify this.

> 
> 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 is not possible. The field is a 2bit field and cannot be greater
than 3. See the RMI_DONATE_BLOCK_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.

We already WARN_ON_ONCE(1), when we get a memreq in a non-memory 
transferring SRO operation. There is nothing we could do in this case
from the generic helper, than to inform the caller that we had to
cancel it.

This is a fundamental misbehavior from an RMM implementation. There are
other ways the RMM could crash the host. e.g., Don't really undelegate
while reporting success. So, I wouldn't care about such pathological
cases.

Cheers
Suzuki


> 
> 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.
>