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

[Yadan Fan]

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.

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

> 
> I don't think what you did is wrong, but I think this is changing the
> intended functionality and you should say so in the commit log.  Right
> now you are saying it doesn't change it, but I think it does - although
> before we were overwriting beyond the array max so I doubt it makes a
> difference and this is obviously better.

Yes, it's a behavior change indeed, I will say so for this in commit log in patch V2.

Thanks,
Yadan