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

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

[David Hildenbrand]

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

[Oscar Salvador]

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

[David Hildenbrand]

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

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

[David Hildenbrand]

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

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

[Oscar Salvador]

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