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