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: <CANn89i+UN32c+-1RdtcuUF0+pDEG5dMGtO41tNZtz4Y=V7f87A@mail.gmail.com>
Date: Tue, 5 Sep 2023 05:49:19 +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 5:41 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> 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.

I will shortly submit this fix for igmpv3_newpack().

It will be useful even if we allow dev->mtu to be in the vicinity of
KMALLOC_MAX_SIZE

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 0c9e768e5628b1c8fd7e87bebe528762ea4a6e1e..418e5fb58fd3f2443f3c88fde5c0776805a832ef
100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -353,8 +353,9 @@ static struct sk_buff *igmpv3_newpack(struct
net_device *dev, unsigned int mtu)
        struct flowi4 fl4;
        int hlen = LL_RESERVED_SPACE(dev);
        int tlen = dev->needed_tailroom;
-       unsigned int size = mtu;
+       unsigned int size;

+       size = min(mtu, IP_MAX_MTU);
        while (1) {
                skb = alloc_skb(size + hlen + tlen,
                                GFP_ATOMIC | __GFP_NOWARN);