Re: [PATCH v17 7/7] firmware: arm_rmm: Add wrappers for Realm related RMI commands

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

On 07/09/2026 11:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] RMI wrapper functions silently discard output registers on SMC call errors, preventing error recovery and leaking memory.
> --
> 
> commit a6c85c25d264d328a0bb8014d88418e614ccef39
> Author: Steven Price <steven.price@arm.com>
> 
> firmware: arm_rmm: Add wrappers for Realm related RMI commands
> 
> Introduce wrappers for the RMI functions needed for creating and managing
> realm guests. This will be used by the KVM to manage the Realms.
> 
>> diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
>> --- a/include/linux/arm-rmi-cmds.h
>> +++ b/include/linux/arm-rmi-cmds.h
> 
> [ ... ]
> 
>> +static inline long rmi_rtt_data_map(unsigned long rd,
> [ ... ]
>> +{
> [ ... ]
>> +	ret = rmi_sro_execute(&regs);
>> +
>> +	if (ret == RMI_SUCCESS && out_top)
>> +		*out_top = regs.a1;
> 
> [Severity: High]
> If the RMM returns an error like RMI_ERROR_RTT partway through, does this
> discard the intermediate progress in regs.a1 by gating it on RMI_SUCCESS?
> 
> If out_top is lost, it seems KVM wouldn't know where the operation stopped,
> making proper error recovery or partial mapping cleanup impossible.
> 
> [ ... ]
> 
>> +static inline long rmi_rtt_data_unmap(unsigned long rd,
> [ ... ]
>> +{
> [ ... ]
>> +	ret = rmi_sro_execute(&regs);
>> +
>> +	if (ret == RMI_SUCCESS) {
>> +		if (out_top)
>> +			*out_top = regs.a1;
>> +		if (out_range)
>> +			*out_range = regs.a2;
>> +		if (out_count)
>> +			*out_count = regs.a3;
>> +	}
> 
> [Severity: High]
> Can this lead to a leak of physical pages?
> 
> When an error occurs (e.g., RMI_ERROR_RTT due to state conflicts), the RMM
> populates the output registers with the number of successfully unmapped
> pages. Because this is gated behind RMI_SUCCESS, losing out_count means KVM
> loses track of how many physical pages were written to the output address
> list.
> 

The output parameters are only valid when the result is RMI_SUCCESS as
described by the success conditions.


> [ ... ]
> 
>> +static inline long rmi_rtt_destroy(unsigned long rd,
> [ ... ]
>> +{
> [ ... ]
>> +	long ret = rmi_sro_execute(&regs);
>> +
>> +	if (ret == RMI_SUCCESS) {
>> +		if (out_rtt)
>> +			*out_rtt = regs.a1;
>> +		if (out_top)
>> +			*out_top = regs.a2;
>> +	}
> 
> [Severity: High]
> Does discarding these outputs on error prevent diagnosing the destruction
> failure?
> 
> If an RMI_ERROR_RTT occurs, the live IPA location is returned in the output
> registers, but it looks like it gets dropped here since the return code is
> not RMI_SUCCESS.

No, the RMM only returns the out_top when the error is RMI_SUCCESS.

> 
> [ ... ]
> 
>> +static inline long rmi_rtt_unprot_unmap(unsigned long rd,
> [ ... ]
>> +{
> [ ... ]
>> +	long ret = rmi_sro_execute(&regs);
>> +
>> +	if (ret == RMI_SUCCESS) {
>> +		if (out_top)
>> +			*out_top = regs.a1;
>> +		if (out_range)
>> +			*out_range = regs.a2;
>> +		if (out_count)
>> +			*out_count = regs.a3;
>> +	}
> 
> [Severity: High]
> Similar to the data unmap wrapper above, does dropping out_count on a
> non-success return code leak physical pages for unprotected memory?
> 


Same here.

Suzuki