[v1] mm/memory_hotplug: prevent accessing by index=-1

[PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by Anastasia Belova 1 year, 8 months ago

nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
array by invalid index with check for nid.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 431b1f6753c0..bb98ee8fe698 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -846,7 +846,7 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
 	unsigned long kernel_early_pages, movable_pages;
 	struct auto_movable_group_stats group_stats = {};
 	struct auto_movable_stats stats = {};
-	pg_data_t *pgdat = NODE_DATA(nid);
+	pg_data_t *pgdat = (nid != NUMA_NO_NODE) ? NODE_DATA(nid) : NULL;
 	struct zone *zone;
 	int i;
 
-- 
2.30.2

Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by David Hildenbrand 1 year, 8 months ago

On 03.06.24 13:28, Anastasia Belova wrote:
> nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
> array by invalid index with check for nid.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>   mm/memory_hotplug.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 431b1f6753c0..bb98ee8fe698 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -846,7 +846,7 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
>   	unsigned long kernel_early_pages, movable_pages;
>   	struct auto_movable_group_stats group_stats = {};
>   	struct auto_movable_stats stats = {};
> -	pg_data_t *pgdat = NODE_DATA(nid);
> +	pg_data_t *pgdat = (nid != NUMA_NO_NODE) ? NODE_DATA(nid) : NULL;
>   	struct zone *zone;
>   	int i;


pgdat is never dereferenced when "nid == NUMA_NO_NODE".

NODE_DATA is defined as

arch/arm64/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
arch/loongarch/include/asm/mmzone.h:#define NODE_DATA(nid)      (node_data[(nid)])
arch/mips/include/asm/mach-ip27/mmzone.h:#define NODE_DATA(n)           (&__node_data[(n)]->pglist)
arch/mips/include/asm/mach-loongson64/mmzone.h:#define NODE_DATA(n)             (__node_data[n])
arch/powerpc/include/asm/mmzone.h:#define NODE_DATA(nid)                (node_data[nid])
arch/riscv/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
arch/s390/include/asm/mmzone.h:#define NODE_DATA(nid) (node_data[nid])
arch/sh/include/asm/mmzone.h:#define NODE_DATA(nid)             (node_data[nid])
arch/sparc/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[nid])
arch/x86/include/asm/mmzone_32.h:#define NODE_DATA(nid) (node_data[nid])
arch/x86/include/asm/mmzone_64.h:#define NODE_DATA(nid)         (node_data[nid])

Regarding architectures that's actually support memory hotplug, this is pure pointer arithmetic.
(it is for mips as well, just less obvious)

So how is that a real problem? Do we have a reproducer?

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by Fedor Pchelkin 1 year, 8 months ago

On Mon, 03. Jun 18:07, David Hildenbrand wrote:
> On 03.06.24 13:28, Anastasia Belova wrote:
> > nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
> > array by invalid index with check for nid.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > 
> > Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >   mm/memory_hotplug.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 431b1f6753c0..bb98ee8fe698 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -846,7 +846,7 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
> >   	unsigned long kernel_early_pages, movable_pages;
> >   	struct auto_movable_group_stats group_stats = {};
> >   	struct auto_movable_stats stats = {};
> > -	pg_data_t *pgdat = NODE_DATA(nid);
> > +	pg_data_t *pgdat = (nid != NUMA_NO_NODE) ? NODE_DATA(nid) : NULL;
> >   	struct zone *zone;
> >   	int i;
> 
> 
> pgdat is never dereferenced when "nid == NUMA_NO_NODE".
> 
> NODE_DATA is defined as
> 
> arch/arm64/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
> arch/loongarch/include/asm/mmzone.h:#define NODE_DATA(nid)      (node_data[(nid)])
> arch/mips/include/asm/mach-ip27/mmzone.h:#define NODE_DATA(n)           (&__node_data[(n)]->pglist)
> arch/mips/include/asm/mach-loongson64/mmzone.h:#define NODE_DATA(n)             (__node_data[n])
> arch/powerpc/include/asm/mmzone.h:#define NODE_DATA(nid)                (node_data[nid])
> arch/riscv/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
> arch/s390/include/asm/mmzone.h:#define NODE_DATA(nid) (node_data[nid])
> arch/sh/include/asm/mmzone.h:#define NODE_DATA(nid)             (node_data[nid])
> arch/sparc/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[nid])
> arch/x86/include/asm/mmzone_32.h:#define NODE_DATA(nid) (node_data[nid])
> arch/x86/include/asm/mmzone_64.h:#define NODE_DATA(nid)         (node_data[nid])

node_data array is declared as follows on most archs:

  struct pglist_data *node_data[MAX_NUMNODES];

It's an array of pointers to struct pglist_data. When doing node_data[-1],
it is actually dereferencing something before the start of the array in
order to obtain a pointer to struct pglist_data, isn't it?

   (C99, 6.5.2.1) The definition of the subscript operator [] is that
   E1[E2] is identical to (*((E1)+(E2))).

> 
> Regarding architectures that's actually support memory hotplug, this is pure pointer arithmetic.
> (it is for mips as well, just less obvious)

Speaking in a dry language of C standard, pointer arithmetic with one
before the first array element is also considered undefined behaviour:

  (C99, 6.5.6p8) "If both the pointer operand and the result point to
  elements of the same array object, or one past the last element of the
  array object, the evaluation shall not produce an overflow; otherwise,
  the behavior is undefined."

Although it doesn't ever crash and probably never won't give any problems,
but who knows what the compiler optimisations would do with this.

> 
> So how is that a real problem? Do we have a reproducer?

This code looks to be executed with memory_hotplug.online_policy=auto-movable,
I suppose it's not a real big problem due to the fact that node_data is a
global variable as otherwise [-1] array access would lead to crashes..

I've triggered the code with node_data[-1] on kernel with UBSAN enabled,
and no splats were observed. Is it due to that node_data is a global
variable or I somehow managed to misuse UBSAN for catching oob access?
Cc'ing linux-hardening.

Nonetheless, maybe it'd be better to define pgdat inside the else-block
in auto_movable_can_online_movable() where it's only used?

Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by David Hildenbrand 1 year, 8 months ago

On 03.06.24 21:54, Fedor Pchelkin wrote:
> On Mon, 03. Jun 18:07, David Hildenbrand wrote:
>> On 03.06.24 13:28, Anastasia Belova wrote:
>>> nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
>>> array by invalid index with check for nid.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
>>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
>>> ---
>>>    mm/memory_hotplug.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 431b1f6753c0..bb98ee8fe698 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -846,7 +846,7 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
>>>    	unsigned long kernel_early_pages, movable_pages;
>>>    	struct auto_movable_group_stats group_stats = {};
>>>    	struct auto_movable_stats stats = {};
>>> -	pg_data_t *pgdat = NODE_DATA(nid);
>>> +	pg_data_t *pgdat = (nid != NUMA_NO_NODE) ? NODE_DATA(nid) : NULL;
>>>    	struct zone *zone;
>>>    	int i;
>>
>>
>> pgdat is never dereferenced when "nid == NUMA_NO_NODE".
>>
>> NODE_DATA is defined as
>>
>> arch/arm64/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
>> arch/loongarch/include/asm/mmzone.h:#define NODE_DATA(nid)      (node_data[(nid)])
>> arch/mips/include/asm/mach-ip27/mmzone.h:#define NODE_DATA(n)           (&__node_data[(n)]->pglist)
>> arch/mips/include/asm/mach-loongson64/mmzone.h:#define NODE_DATA(n)             (__node_data[n])
>> arch/powerpc/include/asm/mmzone.h:#define NODE_DATA(nid)                (node_data[nid])
>> arch/riscv/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
>> arch/s390/include/asm/mmzone.h:#define NODE_DATA(nid) (node_data[nid])
>> arch/sh/include/asm/mmzone.h:#define NODE_DATA(nid)             (node_data[nid])
>> arch/sparc/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[nid])
>> arch/x86/include/asm/mmzone_32.h:#define NODE_DATA(nid) (node_data[nid])
>> arch/x86/include/asm/mmzone_64.h:#define NODE_DATA(nid)         (node_data[nid])
> 
> node_data array is declared as follows on most archs:
> 
>    struct pglist_data *node_data[MAX_NUMNODES];
> 
> It's an array of pointers to struct pglist_data. When doing node_data[-1],
> it is actually dereferencing something before the start of the array in
> order to obtain a pointer to struct pglist_data, isn't it?
> 
>     (C99, 6.5.2.1) The definition of the subscript operator [] is that
>     E1[E2] is identical to (*((E1)+(E2))).
> 

Yes, you are right, I shouldn't have reviewed that on the subway :) I 
thought we'd have a "&" in the front ...

What likely saves us here is the compiler doing the right thing, and not 
actually looking up that pointer when unused -- or even inlining 
auto_movable_can_online_movable() twice into auto_movable_zone_for_pfn().

[...]

> 
> This code looks to be executed with memory_hotplug.online_policy=auto-movable,
> I suppose it's not a real big problem due to the fact that node_data is a
> global variable as otherwise [-1] array access would lead to crashes..
> 
> I've triggered the code with node_data[-1] on kernel with UBSAN enabled,
> and no splats were observed. Is it due to that node_data is a global
> variable or I somehow managed to misuse UBSAN for catching oob access?
> Cc'ing linux-hardening.
> 
> Nonetheless, maybe it'd be better to define pgdat inside the else-block
> in auto_movable_can_online_movable() where it's only used?

Yes, that's cleanest. And that's likely what the compiler does by itself 
already.

Thanks!

-- 
Cheers,

David / dhildenb

[PATCH v2] mm/memory_hotplug: prevent accessing by index=-1

Posted by Anastasia Belova 1 year, 8 months ago

nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
array by invalid index with check for nid.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 mm/memory_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 431b1f6753c0..db78d1b725fc 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -846,7 +846,6 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
 	unsigned long kernel_early_pages, movable_pages;
 	struct auto_movable_group_stats group_stats = {};
 	struct auto_movable_stats stats = {};
-	pg_data_t *pgdat = NODE_DATA(nid);
 	struct zone *zone;
 	int i;
 
@@ -857,6 +856,8 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
 			auto_movable_stats_account_zone(&stats, zone);
 	} else {
 		for (i = 0; i < MAX_NR_ZONES; i++) {
+			pg_data_t *pgdat = NODE_DATA(nid);
+
 			zone = pgdat->node_zones + i;
 			if (populated_zone(zone))
 				auto_movable_stats_account_zone(&stats, zone);
-- 
2.30.2

Re: [PATCH v2] mm/memory_hotplug: prevent accessing by index=-1

Posted by Oscar Salvador 1 year, 8 months ago

On Thu, Jun 06, 2024 at 11:06:59AM +0300, Anastasia Belova wrote:
> nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
> array by invalid index with check for nid.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>

Acked-by: Oscar Salvador <osalvador@suse.de>

Thanks! 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2] mm/memory_hotplug: prevent accessing by index=-1

Posted by David Hildenbrand 1 year, 8 months ago

On 06.06.24 10:06, Anastasia Belova wrote:
> nid may be equal to NUMA_NO_NODE=-1. Prevent accessing node_data
> array by invalid index with check for nid.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: e83a437faa62 ("mm/memory_hotplug: introduce "auto-movable" online policy")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>   mm/memory_hotplug.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 431b1f6753c0..db78d1b725fc 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -846,7 +846,6 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
>   	unsigned long kernel_early_pages, movable_pages;
>   	struct auto_movable_group_stats group_stats = {};
>   	struct auto_movable_stats stats = {};
> -	pg_data_t *pgdat = NODE_DATA(nid);
>   	struct zone *zone;
>   	int i;
>   
> @@ -857,6 +856,8 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
>   			auto_movable_stats_account_zone(&stats, zone);
>   	} else {
>   		for (i = 0; i < MAX_NR_ZONES; i++) {
> +			pg_data_t *pgdat = NODE_DATA(nid);
> +
>   			zone = pgdat->node_zones + i;
>   			if (populated_zone(zone))
>   				auto_movable_stats_account_zone(&stats, zone);

Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by Oscar Salvador 1 year, 8 months ago

On Mon, Jun 03, 2024 at 06:07:39PM +0200, David Hildenbrand wrote:
> pgdat is never dereferenced when "nid == NUMA_NO_NODE".

Right.

> NODE_DATA is defined as
> 
> arch/arm64/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
> arch/loongarch/include/asm/mmzone.h:#define NODE_DATA(nid)      (node_data[(nid)])
> arch/mips/include/asm/mach-ip27/mmzone.h:#define NODE_DATA(n)           (&__node_data[(n)]->pglist)

All look fine, but mips.

Is it not dangerous to try to derefence &__node_data[-1]->pglist?


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

Posted by David Hildenbrand 1 year, 8 months ago

On 03.06.24 19:53, Oscar Salvador wrote:
> On Mon, Jun 03, 2024 at 06:07:39PM +0200, David Hildenbrand wrote:
>> pgdat is never dereferenced when "nid == NUMA_NO_NODE".
> 
> Right.
> 
>> NODE_DATA is defined as
>>
>> arch/arm64/include/asm/mmzone.h:#define NODE_DATA(nid)          (node_data[(nid)])
>> arch/loongarch/include/asm/mmzone.h:#define NODE_DATA(nid)      (node_data[(nid)])
>> arch/mips/include/asm/mach-ip27/mmzone.h:#define NODE_DATA(n)           (&__node_data[(n)]->pglist)
> 
> All look fine, but mips.

$ git grep MEMORY_HOTPLUG | grep mips | wc -l
0

I think it owuld be problematic, if mips would support memory hotplug.

Or am I missing something?

-- 
Cheers,

David / dhildenb