* [PATCH net] net: deal with integer overflows in kmalloc_reserve()
@ 2023-08-31 18:37 Eric Dumazet
2023-09-04 5:58 ` patchwork-bot+netdevbpf
2023-09-04 8:41 ` David Laight
0 siblings, 2 replies; 12+ messages in thread
From: Eric Dumazet @ 2023-08-31 18:37 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, Eric Dumazet, syzbot, Kyle Zeng, Kees Cook,
Vlastimil Babka
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.
kmalloc(0) returns ZERO_SIZE_PTR which is not handled by
skb allocations.
Following trace can be triggered if a netdev->mtu is set
close to 0x7fffffff
We might in the future limit netdev->mtu to more sensible
limit (like KMALLOC_MAX_SIZE).
This patch is based on a syzbot report, and also a report
and tentative fix from Kyle Zeng.
[1]
BUG: KASAN: user-memory-access in __build_skb_around net/core/skbuff.c:294 [inline]
BUG: KASAN: user-memory-access in __alloc_skb+0x3c4/0x6e8 net/core/skbuff.c:527
Write of size 32 at addr 00000000fffffd10 by task syz-executor.4/22554
CPU: 1 PID: 22554 Comm: syz-executor.4 Not tainted 6.1.39-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/03/2023
Call trace:
dump_backtrace+0x1c8/0x1f4 arch/arm64/kernel/stacktrace.c:279
show_stack+0x2c/0x3c arch/arm64/kernel/stacktrace.c:286
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x120/0x1a0 lib/dump_stack.c:106
print_report+0xe4/0x4b4 mm/kasan/report.c:398
kasan_report+0x150/0x1ac mm/kasan/report.c:495
kasan_check_range+0x264/0x2a4 mm/kasan/generic.c:189
memset+0x40/0x70 mm/kasan/shadow.c:44
__build_skb_around net/core/skbuff.c:294 [inline]
__alloc_skb+0x3c4/0x6e8 net/core/skbuff.c:527
alloc_skb include/linux/skbuff.h:1316 [inline]
igmpv3_newpack+0x104/0x1088 net/ipv4/igmp.c:359
add_grec+0x81c/0x1124 net/ipv4/igmp.c:534
igmpv3_send_cr net/ipv4/igmp.c:667 [inline]
igmp_ifc_timer_expire+0x1b0/0x1008 net/ipv4/igmp.c:810
call_timer_fn+0x1c0/0x9f0 kernel/time/timer.c:1474
expire_timers kernel/time/timer.c:1519 [inline]
__run_timers+0x54c/0x710 kernel/time/timer.c:1790
run_timer_softirq+0x28/0x4c kernel/time/timer.c:1803
_stext+0x380/0xfbc
____do_softirq+0x14/0x20 arch/arm64/kernel/irq.c:79
call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:891
do_softirq_own_stack+0x20/0x2c arch/arm64/kernel/irq.c:84
invoke_softirq kernel/softirq.c:437 [inline]
__irq_exit_rcu+0x1c0/0x4cc kernel/softirq.c:683
irq_exit_rcu+0x14/0x78 kernel/softirq.c:695
el0_interrupt+0x7c/0x2e0 arch/arm64/kernel/entry-common.c:717
__el0_irq_handler_common+0x18/0x24 arch/arm64/kernel/entry-common.c:724
el0t_64_irq_handler+0x10/0x1c arch/arm64/kernel/entry-common.c:729
el0t_64_irq+0x1a0/0x1a4 arch/arm64/kernel/entry.S:584
Fixes: 12d6c1d3a2ad ("skbuff: Proactively round up to kmalloc bucket size")
Reported-by: syzbot <syzkaller@googlegroups.com>
Reported-by: Kyle Zeng <zengyhkyle@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
---
net/core/skbuff.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 45707059082f2d706a9820f4484d0d4636aaa930..ba3a0a7f526d7b819d7836d460ea83716b17ce1c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -550,7 +550,7 @@ static void *kmalloc_reserve(unsigned int *size, gfp_t flags, int node,
bool *pfmemalloc)
{
bool ret_pfmemalloc = false;
- unsigned int obj_size;
+ size_t obj_size;
void *obj;
obj_size = SKB_HEAD_ALIGN(*size);
@@ -567,7 +567,13 @@ static void *kmalloc_reserve(unsigned int *size, gfp_t flags, int node,
obj = kmem_cache_alloc_node(skb_small_head_cache, flags, node);
goto out;
}
- *size = obj_size = kmalloc_size_roundup(obj_size);
+
+ obj_size = kmalloc_size_roundup(obj_size);
+ /* The following cast might truncate high-order bits of obj_size, this
+ * is harmless because kmalloc(obj_size >= 2^32) will fail anyway.
+ */
+ *size = (unsigned int)obj_size;
+
/*
* Try a regular allocation, when that fails and we're not entitled
* to the reserves, fail.
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-08-31 18:37 [PATCH net] net: deal with integer overflows in kmalloc_reserve() Eric Dumazet
@ 2023-09-04 5:58 ` patchwork-bot+netdevbpf
2023-09-04 8:41 ` David Laight
1 sibling, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-04 5:58 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, kuba, pabeni, netdev, eric.dumazet, syzkaller, zengyhkyle,
keescook, vbabka
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Thu, 31 Aug 2023 18:37:50 +0000 you wrote:
> Blamed commit changed:
> ptr = kmalloc(size);
> if (ptr)
> size = ksize(ptr);
>
> to:
> size = kmalloc_size_roundup(size);
> ptr = kmalloc(size);
>
> [...]
Here is the summary with links:
- [net] net: deal with integer overflows in kmalloc_reserve()
https://git.kernel.org/netdev/net/c/915d975b2ffa
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-08-31 18:37 [PATCH net] net: deal with integer overflows in kmalloc_reserve() Eric Dumazet
2023-09-04 5:58 ` patchwork-bot+netdevbpf
@ 2023-09-04 8:41 ` David Laight
2023-09-04 9:06 ` Eric Dumazet
1 sibling, 1 reply; 12+ messages in thread
From: David Laight @ 2023-09-04 8:41 UTC (permalink / raw)
To: 'Eric Dumazet', David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, syzbot, Kyle Zeng, Kees Cook, Vlastimil Babka
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.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-04 8:41 ` David Laight
@ 2023-09-04 9:06 ` Eric Dumazet
2023-09-04 9:27 ` David Laight
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2023-09-04 9:06 UTC (permalink / raw)
To: David Laight
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kyle Zeng, Kees Cook, Vlastimil Babka
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()
I personally can not test 32bit kernels.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-04 9:06 ` Eric Dumazet
@ 2023-09-04 9:27 ` David Laight
2023-09-04 23:49 ` Kyle Zeng
0 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2023-09-04 9:27 UTC (permalink / raw)
To: 'Eric Dumazet'
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kyle Zeng, Kees Cook, Vlastimil Babka
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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-04 9:27 ` David Laight
@ 2023-09-04 23:49 ` Kyle Zeng
2023-09-05 3:41 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Kyle Zeng @ 2023-09-04 23:49 UTC (permalink / raw)
To: David Laight
Cc: 'Eric Dumazet',
David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
[-- 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-04 23:49 ` Kyle Zeng
@ 2023-09-05 3:41 ` Eric Dumazet
2023-09-05 3:49 ` Eric Dumazet
2023-09-05 8:36 ` David Laight
0 siblings, 2 replies; 12+ messages in thread
From: Eric Dumazet @ 2023-09-05 3:41 UTC (permalink / raw)
To: Kyle Zeng
Cc: David Laight, David S . Miller, Jakub Kicinski, Paolo Abeni,
netdev, eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
On Tue, Sep 5, 2023 at 1:49 AM Kyle Zeng <zengyhkyle@gmail.com> wrote:
>
> 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.
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-05 3:41 ` Eric Dumazet
@ 2023-09-05 3:49 ` Eric Dumazet
2023-09-05 8:36 ` David Laight
1 sibling, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2023-09-05 3:49 UTC (permalink / raw)
To: Kyle Zeng
Cc: David Laight, David S . Miller, Jakub Kicinski, Paolo Abeni,
netdev, eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
On Tue, Sep 5, 2023 at 5:41 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 5, 2023 at 1:49 AM Kyle Zeng <zengyhkyle@gmail.com> wrote:
> >
> > 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.
>
> 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);
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-05 3:41 ` Eric Dumazet
2023-09-05 3:49 ` Eric Dumazet
@ 2023-09-05 8:36 ` David Laight
2023-09-05 12:27 ` Eric Dumazet
1 sibling, 1 reply; 12+ messages in thread
From: David Laight @ 2023-09-05 8:36 UTC (permalink / raw)
To: 'Eric Dumazet', Kyle Zeng
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
From: Eric Dumazet
> Sent: 05 September 2023 04:42
...
> 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
kmem_alloc_reserve() also needs fixing.
It's purpose is to find the size that kmem_alloc() will
allocated so that the full size can be allocated rather
than later finding out the allocated size.
The latter has issues with the compiler (etc) tracking
the sizes of allocates.
So it must never return a smaller size.
Whether the path that returns 0 can happen or not
the correct error return is the original size.
The later kmalloc() will then probably fail and
be checked for.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-05 8:36 ` David Laight
@ 2023-09-05 12:27 ` Eric Dumazet
2023-09-05 12:34 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2023-09-05 12:27 UTC (permalink / raw)
To: David Laight
Cc: Kyle Zeng, David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
On Tue, Sep 5, 2023 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Eric Dumazet
> > Sent: 05 September 2023 04:42
> ...
> > 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
>
> kmem_alloc_reserve() also needs fixing.
Yes, this is what I said. Please provide a patch ?
I stopped caring about 32bit 10 years ago.
> It's purpose is to find the size that kmem_alloc() will
> allocated so that the full size can be allocated rather
> than later finding out the allocated size.
> The latter has issues with the compiler (etc) tracking
> the sizes of allocates.
>
> So it must never return a smaller size.
> Whether the path that returns 0 can happen or not
> the correct error return is the original size.
> The later kmalloc() will then probably fail and
> be checked for.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-05 12:27 ` Eric Dumazet
@ 2023-09-05 12:34 ` Eric Dumazet
2023-09-05 12:44 ` David Laight
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2023-09-05 12:34 UTC (permalink / raw)
To: David Laight
Cc: Kyle Zeng, David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
On Tue, Sep 5, 2023 at 2:27 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 5, 2023 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Eric Dumazet
> > > Sent: 05 September 2023 04:42
> > ...
> > > 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
> >
> > kmem_alloc_reserve() also needs fixing.
>
> Yes, this is what I said. Please provide a patch ?
Oops, I thought you were speaking about kmalloc_size_roundup()
kmalloc_reserve() is fine, all overflows must be taken care of before
reaching it.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net] net: deal with integer overflows in kmalloc_reserve()
2023-09-05 12:34 ` Eric Dumazet
@ 2023-09-05 12:44 ` David Laight
0 siblings, 0 replies; 12+ messages in thread
From: David Laight @ 2023-09-05 12:44 UTC (permalink / raw)
To: 'Eric Dumazet'
Cc: Kyle Zeng, David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Kees Cook, Vlastimil Babka
From: Eric Dumazet
> Sent: 05 September 2023 13:34
>
> On Tue, Sep 5, 2023 at 2:27 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Sep 5, 2023 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Eric Dumazet
> > > > Sent: 05 September 2023 04:42
> > > ...
> > > > 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
> > >
> > > kmem_alloc_reserve() also needs fixing.
> >
> > Yes, this is what I said. Please provide a patch ?
>
> Oops, I thought you were speaking about kmalloc_size_roundup()
>
> kmalloc_reserve() is fine, all overflows must be taken care of before
> reaching it.
I was talking about roundup()
I'm in all the wrong place to generate a patch.
And having to send the emails from windows makes the procedure a
right PITA.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-09-05 12:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-31 18:37 [PATCH net] net: deal with integer overflows in kmalloc_reserve() Eric Dumazet
2023-09-04 5:58 ` patchwork-bot+netdevbpf
2023-09-04 8:41 ` David Laight
2023-09-04 9:06 ` Eric Dumazet
2023-09-04 9:27 ` David Laight
2023-09-04 23:49 ` Kyle Zeng
2023-09-05 3:41 ` Eric Dumazet
2023-09-05 3:49 ` Eric Dumazet
2023-09-05 8:36 ` David Laight
2023-09-05 12:27 ` Eric Dumazet
2023-09-05 12:34 ` Eric Dumazet
2023-09-05 12:44 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).