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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 3 Jun 2024 22:15:21 +0200
From: David Hildenbrand <david@...hat.com>
To: Fedor Pchelkin <pchelkin@...ras.ru>
Cc: Anastasia Belova <abelova@...ralinux.ru>, lvc-project@...uxtesting.org,
 linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 linux-kernel@...r.kernel.org, Oscar Salvador <osalvador@...e.de>,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1

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@...ralinux.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