netdev - Re: [PATCH net] net: deal with integer overflows in kmalloc

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZPZtBWm06f321Tp/@westworld>
Date: Mon, 4 Sep 2023 16:49:25 -0700
From: Kyle Zeng <zengyhkyle@...il.com>
To: David Laight <David.Laight@...lab.com>
Cc: 'Eric Dumazet' <edumazet@...gle.com>,
	"David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"eric.dumazet@...il.com" <eric.dumazet@...il.com>,
	syzbot <syzkaller@...glegroups.com>,
	Kees Cook <keescook@...omium.org>, Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()

On Mon, Sep 04, 2023 at 09:27:28AM +0000, David Laight wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> > Sent: 04 September 2023 10:06
> > To: David Laight <David.Laight@...LAB.COM>
> > Cc: David S . Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> > <pabeni@...hat.com>; netdev@...r.kernel.org; eric.dumazet@...il.com; syzbot
> > <syzkaller@...glegroups.com>; Kyle Zeng <zengyhkyle@...il.com>; Kees Cook <keescook@...omium.org>;
> > Vlastimil Babka <vbabka@...e.cz>
> > Subject: Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
> > 
> > On Mon, Sep 4, 2023 at 10:41 AM David Laight <David.Laight@...lab.com> wrote:
> > >
> > > From: Eric Dumazet
> > > > Sent: 31 August 2023 19:38
> > > >
> > > > Blamed commit changed:
> > > >     ptr = kmalloc(size);
> > > >     if (ptr)
> > > >       size = ksize(ptr);
> > > >
> > > > to:
> > > >     size = kmalloc_size_roundup(size);
> > > >     ptr = kmalloc(size);
> > > >
> > > > This allowed various crash as reported by syzbot [1]
> > > > and Kyle Zeng.
> > > >
> > > > Problem is that if @size is bigger than 0x80000001,
> > > > kmalloc_size_roundup(size) returns 2^32.
> > > >
> > > > kmalloc_reserve() uses a 32bit variable (obj_size),
> > > > so 2^32 is truncated to 0.
> > >
> > > Can this happen on 32bit arch?
> > > In that case kmalloc_size_roundup() will return 0.
> > 
> > Maybe, but this would be a bug in kmalloc_size_roundup()
> 
> That contains:
> 	/* Short-circuit saturated "too-large" case. */
> 	if (unlikely(size == SIZE_MAX))
> 		return SIZE_MAX;
> 
> It can also return 0 on failure, I can't remember if kmalloc(0)
> is guaranteed to be NULL (malloc(0) can do 'other things').
> 
> Which is entirely hopeless since MAX_SIZE is (size_t)-1.
> 
> IIRC kmalloc() has a size limit (max 'order' of pages) so
> kmalloc_size_roundup() ought check for that (or its max value).
> 
> The final:
> 	/* The flags don't matter since size_index is common to all. */
> 	c = kmalloc_slab(size, GFP_KERNEL);
> 	return c ? c->object_size : 0;
> probably ought to return size if c is even NULL.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

> It can also return 0 on failure, I can't remember if kmalloc(0)
> is guaranteed to be NULL (malloc(0) can do 'other things').
kmalloc(0) returns ZERO_SIZE_PTR (16).

My proposed patch is to check the return value of kmalloc, making sure it is neither NULL or ZERO_SIZE_PTR. The patch is attached. It should work for both 32bit and 64bit systems.

View attachment "0001-properly-check-for-integer-overflow-in-__alloc_skb.patch" of type "text/x-diff" (1072 bytes)