Re: MIPS: mm: Fix out-of-bounds write in maar_res_walk()

[Yadan Fan]

Hi Thomas,

On 5/26/26 14:57, Thomas Bogendoerfer wrote:
> On Tue, May 26, 2026 at 08:51:25AM +0800, Yadan Fan wrote:
>> Hi Liam,
>>
>> On 5/25/26 22:15, Liam R. Howlett wrote:
>>> On 26/05/25 07:06PM, Yadan Fan wrote:
>>>> maar_res_walk() uses wi->num_cfg as the index into the fixed-size
>>>> wi->cfg array, but checks whether the array is full only after it has
>>>> filled the selected entry. If walk_system_ram_range() reports more than
>>>> 16 memory ranges, the overflow call writes one struct maar_config past
>>>> the end of the array before WARN_ON() prevents num_cfg from advancing.
>>>>
>>>> Move the full-array check before taking the array slot. Keep the
>>>> existing behavior of warning and returning 0 when the scratch array has
>>>> no room left.
>>>>
>>>> Fixes: a5718fe8f70f ("MIPS: mm: Drop boot_mem_map")
>>>> Signed-off-by: Yadan Fan <ydfan@xxxxxxxx>
>>>> ---
>>>>  arch/mips/mm/init.c | 12 ++++++++----
>>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/mips/mm/init.c b/arch/mips/mm/init.c
>>>> index 55b25e85122a..0ba958b7ffa5 100644
>>>> --- a/arch/mips/mm/init.c
>>>> +++ b/arch/mips/mm/init.c
>>>> @@ -272,9 +272,15 @@ static int maar_res_walk(unsigned long start_pfn, unsigned long nr_pages,
>>>>  			 void *data)
>>>>  {
>>>>  	struct maar_walk_info *wi = data;
>>>> -	struct maar_config *cfg = &wi->cfg[wi->num_cfg];
>>>> +	struct maar_config *cfg;
>>>>  	unsigned int maar_align;
>>>>  
>>>> +	/* Ensure we don't overflow the cfg array */
>>>> +	if (WARN_ON(wi->num_cfg >= ARRAY_SIZE(wi->cfg)))
>>>> +		return 0;
>>>
>>> We should probably change this to WARN_ON_ONCE() if we're moving it?
>>
>> Yes, it should be WARN_ON_ONCE() to avoid repeated warnings later once wi->num_cfg reached ARRAY_SIZE(wi->cfg),
>> I will change it in patch V2.
> 
> no need for that, just return -1 in the error case, this will terminate
> the walk

Right, I will change this in v3.

> 
>>>
>>>> +
>>>> +	cfg = &wi->cfg[wi->num_cfg];
>>>> +
>>>>  	/* MAAR registers hold physical addresses right shifted by 4 bits */
>>>>  	maar_align = BIT(MIPS_MAAR_ADDR_SHIFT + 4);
>>>>  
>>>> @@ -283,9 +289,7 @@ static int maar_res_walk(unsigned long start_pfn, unsigned long nr_pages,
>>>>  	cfg->upper = ALIGN_DOWN(PFN_PHYS(start_pfn + nr_pages), maar_align) - 1;
>>>>  	cfg->attrs = MIPS_MAAR_S;
>>>>  
>>>> -	/* Ensure we don't overflow the cfg array */
>>>> -	if (!WARN_ON(wi->num_cfg >= ARRAY_SIZE(wi->cfg)))
>>>> -		wi->num_cfg++;
>>>> +	wi->num_cfg++;
>>>
>>> AFAICT, this is the only place num_cfg is incremented.  Since we are
>>> trying to avoid overflow, I think the initial logic is flawed in that we
>>> do trigger too late - we can write one beyond the array index max (to
>>> the array size).
>>>
>>> But what you have done changes the functionality in a subtle way - we
>>> won't keep overwriting the last entry (as, I think was the initial
>>> intent here?)
>>
>> Yes.
> 
> no it wasn't. Before the switch to maas_res_walk the to be filled array
> was exactly the needed size. Now we don't know how many entries there
> are, but the limiting point is the numer of configured maar entries,
> which might be even less than the 16 entries of the array. So we have
> to deal with this problem anyway.
> 
> Thomas.
> 

Thanks,
Yadan