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: <CANn89iJDsm-xE4K2_BWngOQeuhOFmOhwVfk5=sszf0E+3UcH=g@mail.gmail.com>
Date: Tue, 5 Sep 2023 05:41:42 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Kyle Zeng <zengyhkyle@...il.com>
Cc: David Laight <David.Laight@...lab.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 Tue, Sep 5, 2023 at 1:49 AM Kyle Zeng <zengyhkyle@...il.com> wrote:
>
> 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.

Again, I do not want this patch, I want to fix the root cause(s).

It makes no sense to allow dev->mtu to be as big as 0x7fffffff and
ultimately allow size to be bigger than 0x80000000
I prefer waiting for net-next to open first.

Adding code in the fast path for something that must not ever happen is a no go.

Only CONFIG_DEBUG_NET=y stuff could be accepted.