Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()

[Kyle Zeng <zengyhkyle@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

On Mon, Sep 04, 2023 at 09:27:28AM +0000, David Laight wrote:
> From: Eric Dumazet <edumazet@google.com>
> > Sent: 04 September 2023 10:06
> > To: David Laight <David.Laight@ACULAB.COM>
> > Cc: David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > <pabeni@redhat.com>; netdev@vger.kernel.org; eric.dumazet@gmail.com; syzbot
> > <syzkaller@googlegroups.com>; Kyle Zeng <zengyhkyle@gmail.com>; Kees Cook <keescook@chromium.org>;
> > Vlastimil Babka <vbabka@suse.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@aculab.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.

[-- Attachment #2: 0001-properly-check-for-integer-overflow-in-__alloc_skb.patch --]
[-- Type: text/x-diff, Size: 1070 bytes --]

From 034ac600c639bfa93c54e317ea3712538c749de6 Mon Sep 17 00:00:00 2001
From: Kyle Zeng <zengyhkyle@gmail.com>
Date: Mon, 28 Aug 2023 17:53:40 -0700
Subject: [PATCH] properly check for integer overflow in __alloc_skb

kmalloc_reserve may return ZERO_SIZE_PTR(0x10) when integer overflow
happens since size is controlled by users.
Make sure we handle this case properly.

Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a298992060e..f219fef5a16 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -642,7 +642,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	 * Both skb->head and skb_shared_info are cache line aligned.
 	 */
 	data = kmalloc_reserve(&size, gfp_mask, node, &pfmemalloc);
-	if (unlikely(!data))
+	if (unlikely(ZERO_OR_NULL_PTR(data)))
 		goto nodata;
 	/* kmalloc_size_roundup() might give us more room than requested.
 	 * Put skb_shared_info exactly at the end of allocated zone,
-- 
2.34.1