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@xxxxxxxxxxxxx>
---
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
