* Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1 [not found] <20240603112830.7432-1-abelova@astralinux.ru> @ 2024-06-03 16:07 ` David Hildenbrand 2024-06-03 17:53 ` Oscar Salvador [not found] ` <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru> 0 siblings, 2 replies; 5+ messages in thread From: David Hildenbrand @ 2024-06-03 16:07 UTC (permalink / raw) To: Anastasia Belova Cc: Oscar Salvador, Andrew Morton, linux-mm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1 2024-06-03 16:07 ` [PATCH] mm/memory_hotplug: prevent accessing by index=-1 David Hildenbrand @ 2024-06-03 17:53 ` Oscar Salvador 2024-06-03 17:58 ` David Hildenbrand [not found] ` <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru> 1 sibling, 1 reply; 5+ messages in thread From: Oscar Salvador @ 2024-06-03 17:53 UTC (permalink / raw) To: David Hildenbrand Cc: Anastasia Belova, Andrew Morton, linux-mm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1 2024-06-03 17:53 ` Oscar Salvador @ 2024-06-03 17:58 ` David Hildenbrand 0 siblings, 0 replies; 5+ messages in thread From: David Hildenbrand @ 2024-06-03 17:58 UTC (permalink / raw) To: Oscar Salvador Cc: Anastasia Belova, Andrew Morton, linux-mm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru>]
* Re: [PATCH] mm/memory_hotplug: prevent accessing by index=-1 [not found] ` <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru> @ 2024-06-03 20:15 ` David Hildenbrand [not found] ` <20240606080659.18525-1-abelova@astralinux.ru> 0 siblings, 1 reply; 5+ messages in thread From: David Hildenbrand @ 2024-06-03 20:15 UTC (permalink / raw) To: Fedor Pchelkin Cc: Anastasia Belova, lvc-project, linux-mm, Andrew Morton, linux-kernel, Oscar Salvador, linux-hardening 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20240606080659.18525-1-abelova@astralinux.ru>]
* Re: [PATCH v2] mm/memory_hotplug: prevent accessing by index=-1 [not found] ` <20240606080659.18525-1-abelova@astralinux.ru> @ 2024-06-07 7:34 ` Oscar Salvador 0 siblings, 0 replies; 5+ messages in thread From: Oscar Salvador @ 2024-06-07 7:34 UTC (permalink / raw) To: Anastasia Belova Cc: David Hildenbrand, Andrew Morton, linux-mm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-06-07 7:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20240603112830.7432-1-abelova@astralinux.ru>
2024-06-03 16:07 ` [PATCH] mm/memory_hotplug: prevent accessing by index=-1 David Hildenbrand
2024-06-03 17:53 ` Oscar Salvador
2024-06-03 17:58 ` David Hildenbrand
[not found] ` <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru>
2024-06-03 20:15 ` David Hildenbrand
[not found] ` <20240606080659.18525-1-abelova@astralinux.ru>
2024-06-07 7:34 ` [PATCH v2] " Oscar Salvador
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox