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

[Thomas Bogendoerfer]

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

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

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]