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 <[email protected]>
---
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;
--
2.30.2
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 <[email protected]>
> ---
> 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
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
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
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 <[email protected]>
> > ---
> > 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))).
>
> Regarding architectures that's actually support memory hotplug, this is pure pointer arithmetic.
> (it is for mips as well, just less obvious)
Speaking in a dry language of C standard, pointer arithmetic with one
before the first array element is also considered undefined behaviour:
(C99, 6.5.6p8) "If both the pointer operand and the result point to
elements of the same array object, or one past the last element of the
array object, the evaluation shall not produce an overflow; otherwise,
the behavior is undefined."
Although it doesn't ever crash and probably never won't give any problems,
but who knows what the compiler optimisations would do with this.
>
> So how is that a real problem? Do we have a reproducer?
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?
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
---
mm/memory_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 431b1f6753c0..db78d1b725fc 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -846,7 +846,6 @@ 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);
struct zone *zone;
int i;
@@ -857,6 +856,8 @@ static bool auto_movable_can_online_movable(int nid, struct memory_group *group,
auto_movable_stats_account_zone(&stats, zone);
} else {
for (i = 0; i < MAX_NR_ZONES; i++) {
+ pg_data_t *pgdat = NODE_DATA(nid);
+
zone = pgdat->node_zones + i;
if (populated_zone(zone))
auto_movable_stats_account_zone(&stats, zone);
--
2.30.2
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 <[email protected]>
Acked-by: Oscar Salvador <[email protected]>
Thanks!
--
Oscar Salvador
SUSE Labs