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

[sashiko-bot@kernel.org]

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.

[ ... ]

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

[ ... ]

> +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?

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