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-next>] [day] [month] [year] [list]
Message-ID: <20240603-4f7a5fd957aa1f9cbc8d5f14-pchelkin@ispras.ru>
Date: Mon, 3 Jun 2024 22:54:47 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: David Hildenbrand <david@...hat.com>
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 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))).

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ