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

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

On 30/09/2026 09:45, Suzuki K Poulose wrote:
> 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.

I have folded in the following hunk:

diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
index 8d839e9e61424..12075679863c4 100644
--- a/drivers/firmware/arm_rmm/rmi.c
+++ b/drivers/firmware/arm_rmm/rmi.c
@@ -354,7 +354,7 @@ static int rmi_sro_donate_contig(struct 
rmi_sro_state *sro,
                           0, out_regs);
         donated_size = out_regs->a1 << PAGE_SHIFT;

-       if (WARN_ON(donated_size > size))
+       if (WARN_ON(out_regs->a1 > (size >> PAGE_SHIFT)))
                 donated_size = size;

         /* All granules consumed by the RMM */


Suzuki