Re: [PATCH v18 6/7] firmware: arm_rmm: Ensure the RMM has GPT entries for memory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing forward-progress check on firmware-provided boundaries (`next`) can cause an infinite loop during boot or memory hotplug.
- [High] Sub-page or inverted memblock regions are incorrectly expanded to 1GB blocks, resulting in out-of-bounds firmware queries that can falsely fail RMI initialization.
--

commit 9d5a0191d0b2d820e7529339f354ee6f7cd2d469
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Ensure the RMM has GPT entries for memory

This patch adds checks during initialization and hotplug to ensure that the
firmware correctly tracks memory at fine granularity and manages L1 GPTs. If
the firmware expects the host to manage tracking or GPT memory, the RMM is
deactivated.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index ecc89e91d264d..583e1aca9b15a 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c

[ ... ]

> +static int rmi_verify_memory_tracking(phys_addr_t start, phys_addr_t end)
> +{
> +	while (start < end) {
> +		unsigned long ret, category, state, next;
> +
> +		ret = rmi_granule_tracking_get(start, end, &category, &state, &next);
> +		if (ret != RMI_SUCCESS)
> +			return -ENOMEM;
> +
> +		if (state != RMI_TRACKING_FINE ||
> +		    category != RMI_MEM_CATEGORY_CONVENTIONAL) {
> +			/* TODO: Set granule tracking in this case */
> +			pr_err("Granule tracking for region isn't fine/conventional: %llx-%lx\n",
> +			       start, next);
> +			return -ENODEV;
> +		}
> +		start = next;
                ^^^^^^^^^^^^

[Severity: High]
Are we missing a forward-progress check here?

If a buggy or malicious firmware returns a next boundary where next <= start,
this loop will never terminate, resulting in a permanent kernel hang during
boot or memory hotplug.

The pre-existing rmi_undelegate_range() handles this exact scenario with:

    if (WARN_ON(out_top <= phys))
        return -ENXIO;

Could we add a similar validation check here to verify next > start?

> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * We do not support creating L1 GPTs yet. So, make sure that
> + * all the regions are managed by the firmware.
> + */
> +static int rmi_verify_gpt_firmware_managed(phys_addr_t start, phys_addr_t end)
> +{
> +	unsigned long l0gpt_sz;
> +	unsigned long next, par_state;
> +
> +	l0gpt_sz = 1UL << (30 + FIELD_GET(RMI_FEATURE_REGISTER_1_L0GPTSZ,
> +					  rmi_feat_reg(1)));
> +	start = ALIGN_DOWN(start, l0gpt_sz);
> +	end = ALIGN(end, l0gpt_sz);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this alignment expansion cause issues if the incoming range is inverted?

As noted below in rmi_init_metadata(), if an inverted range (where
start > end) is passed in, this alignment expands it into a massive block.

For example, if start=0x2000 and end=0x1000, and l0gpt_sz is 1GB, this expands
to start=0 and end=1GB. The start < end loop condition then evaluates to true,
and we end up querying the firmware for a 1GB block that might not be managed
by firmware, falsely failing initialization.

> +
> +	while (start < end) {
> +		long ret = rmi_gpt_info(start, end, &next, &par_state);
> +
> +		if (ret != RMI_SUCCESS)
> +			return -ENOMEM;
> +
> +		if (par_state != RMI_GPT_PAR_PLAT) {
> +			pr_err("GPT for the region is not managed by firmware %llx-%lx\n",
> +				start, next);
> +			return -ENOMEM;
> +		}
> +		start = next;
                ^^^^^^^^^^^^

[Severity: High]
Similar to rmi_verify_memory_tracking(), is a forward-progress check
needed here to prevent an infinite loop if the firmware returns a boundary
where next <= start?

> +	}
> +
> +	return 0;
> +}
> +
> +static int rmi_prepare_memory(phys_addr_t start, phys_addr_t end)
> +{
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Should there be an if (start >= end) return 0; check here?

This would protect downstream logic and prevent empty or inverted regions from
reaching the alignment expansion in rmi_verify_gpt_firmware_managed().

> +	int ret;
> +
> +	ret = rmi_verify_memory_tracking(start, end);
> +	if (ret)
> +		return ret;
> +
> +	return rmi_verify_gpt_firmware_managed(start, end);
> +}
> +
> +static int rmi_init_metadata(void)
> +{
> +	phys_addr_t start, end;
> +	struct memblock_region *r;
> +
> +	for_each_mem_region(r) {
> +		int ret;
> +
> +		/* Firmware-reserved NOMAP regions are not usable system RAM */
> +		if (memblock_is_nomap(r))
> +			continue;
> +
> +		start = memblock_region_memory_base_pfn(r) << PAGE_SHIFT;
> +		end = memblock_region_memory_end_pfn(r) << PAGE_SHIFT;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this calculate an inverted range (start > end) for unaligned memblock
regions smaller than a page?

memblock_region_memory_base_pfn() rounds up, while
memblock_region_memory_end_pfn() rounds down. For small, unaligned memory
regions, this yields start > end.

This inverted range then flows into rmi_prepare_memory() and gets
incorrectly expanded to a full 1GB block in
rmi_verify_gpt_firmware_managed(), leading to a fatal initialization failure.

> +
> +		ret = rmi_prepare_memory(start, end);
> +		if (ret)
> +			return ret;
> +	}

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