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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm_rmm: spurious SRO cancellation when contig donate cache fills up
- [Medium] arm_rmm: infinite loop in SRO execution if RMM consumes zero granules
--

commit 11de55b0945f49bf7651f24a6bc346b286810815
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Add support for SRO

The commit introduces support for Stateful RMI Operations (SRO) in RMM v2.0.
It adds logic to handle memory donate and reclaim operations, allowing SMCs
to return in progress and track state using an rmi_sro_state structure.
Wrappers for SRO RMI commands are also provided to integrate with the stateful
command infrastructure.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index b558965a05a2c..7d4e04a5d7c0a 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c
[ ... ]
> +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)
> +{
[ ... ]
> +	/* Reuse the cached address range if we have one */
> +	for (int i = 0; i < sro->addr_count; i++) {
> +		unsigned long entry = sro->addr_list[i];
> +
> +		if (RMI_ADDR_RANGE_BLOCK_SIZE(entry) == block_size_fld &&
> +		    RMI_ADDR_RANGE_COUNT(entry) == count &&
> +		    RMI_ADDR_RANGE_STATE(entry) == state &&
> +		    IS_ALIGNED(RMI_ADDR_RANGE_ADDR(entry), size)) {
> +			sro->addr_count--;
> +			swap(sro->addr_list[sro->addr_count],
> +			     sro->addr_list[i]);
> +
> +			goto mem_donate;
> +		}
> +	}
> +
> +	ret = rmi_sro_ensure_capacity(sro, 1);
> +	if (ret)
> +		return ret;

[Severity: Medium]
When the cache fills up with unconsumed fragments, rmi_sro_ensure_capacity()
returns -ENOSPC. Does returning the error here cause a valid SRO command to be
spuriously cancelled?

Unlike rmi_sro_donate_noncontig() which handles a full cache by calling
rmi_sro_free() to flush the unconsumed fragments, the contiguous path simply
bails out. Could this be triggered if the firmware requests memory in patterns
that fragment the cache?

> +
> +	virt = alloc_pages_exact(size, gfp);
> +	if (!virt)
> +		return -ENOMEM;
[ ... ]
> +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(out_regs->a1 > (size >> PAGE_SHIFT)))
> +		donated_size = size;
> +
> +	/* All granules consumed by the RMM */
> +	if (donated_size == size)
> +		return 0;
> +	/* No granules were consumed by the RMM, cache them */
> +	if (donated_size == 0) {
> +		sro->addr_count++;
> +		return 0;
> +	}

[Severity: Medium]
If a buggy or interrupted RMM consumes 0 granules, does caching the block and
returning 0 here create an infinite loop?

Because 0 is a success return code, the execution loop in
rmi_sro_memxfer_execute() will skip the cancellation logic and re-issue the
same request, as shown below:

> +
> +	/* The granules were partially consumed, reclaim the unused ones. */
> +	free_addr_range(sro->addr_list[sro->addr_count], donated_size);
> +
> +	return 0;
> +}
[ ... ]
> +long rmi_sro_memxfer_execute(struct rmi_sro_state *sro, gfp_t gfp)
> +{
> +	struct arm_smccc_1_2_regs *regs = &sro->regs;
> +	bool cancelled = false;
> +	unsigned long sro_handle;
> +
> +	rmi_smccc_invoke(regs);
> +
> +	sro_handle = regs->a1;
> +	while (RMI_RESULT_STATUS(regs->a0) == RMI_INCOMPLETE) {
> +		bool can_cancel = RMI_RESULT_CAN_CANCEL(regs->a0) == RMI_OP_CAN_CANCEL;
> +		int ret = 0;
> +
> +		switch (RMI_RESULT_MEMREQ(regs->a0)) {
> +		case RMI_OP_MEM_REQ_NONE:
> +			rmi_op_continue(sro_handle, RMI_CONTINUE_KEEP_GOING,
> +					regs);
> +			break;
> +		case RMI_OP_MEM_REQ_DONATE:
> +			ret = rmi_sro_donate(sro, sro_handle, regs->a2, regs,
> +					     gfp);
> +			break;

If ret is 0, the error handling below is skipped, causing the while loop
to re-execute without progress. While firmware is trusted, could we hang the
CPU indefinitely here, bypassing the loop cancellation mechanisms?

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