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

[Liam R. Howlett]

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?

> +
> +	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?)

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.

>  
>  	return 0;
>  }
> -- 
> 2.51.0