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

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

[Yadan Fan]

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@suse.com>
---
 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;
+
+	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++;
 
 	return 0;
 }
-- 
2.51.0

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@suse.com>
> ---
>  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

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@suse.com>
>> ---
>>  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

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@suse.com>
> >> ---
> >>  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 ]

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@suse.com>
>>>> ---
>>>>  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