Message-ID: <CANn89iK0nCQFgA_2UaN4857fQKip0tsraCEQGhF=h8RJKKJnPw@mail.gmail.com>
Date: Wed, 1 May 2024 17:05:59 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, kernel-dev@...lia.com
Subject: Re: [PATCH] net: fix out-of-bounds access in ops_init

On Tue, Apr 30, 2024 at 4:01 PM Thadeu Lima de Souza Cascardo
<cascardo@...lia.com> wrote:
>
> On Tue, Apr 30, 2024 at 11:13:51AM +0200, Eric Dumazet wrote:
> > On Tue, Apr 30, 2024 at 10:43 AM Thadeu Lima de Souza Cascardo
> > <cascardo@...lia.com> wrote:
> > >
> > > net_alloc_generic is called by net_alloc, which is called without any
> > > locking. It reads max_gen_ptrs, which is changed under pernet_ops_rwsem. It
> > > is read twice, first to allocate an array, then to set s.len, which is
> > > later used to limit the bounds of the array access.
> > >
> > > It is possible that the array is allocated and another thread is
> > > registering a new pernet ops, increments max_gen_ptrs, which is then used
> > > to set s.len with a larger than allocated length for the variable array.
> > >
> > > Fix it by delaying the allocation to setup_net, which is always called
> > > under pernet_ops_rwsem, and is called right after net_alloc.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
> >
> > Good catch !
> >
> > Could you provide a Fixes: tag ?
> >
>
> Sorry I didn't include it at first. That would be:
>
> Fixes: 073862ba5d24 ("netns: fix net_alloc_generic()")
>
> > Have you considered reading max_gen_ptrs once in net_alloc_generic() ?
> > This would make the patch a little less complicated.
> >
>
> It would look like this "v2" below.
>
> One of the things that may have crossed my mind is that in case of a race, and
> max_gen_ptrs is incremented before setup_net is called, it would have to be
> reallocated anyway. Though this would be uncommon, that gave me the idea to
> implement the solution as I submitted. It seemed easier to get right, instead
> of messing around the memory model. :-)
>
> But even if there is a race and we get the value wrong, setup_net will
> reallocate it, so it should all be fine as long as we use the same value for
> the generic_size calculation and s.len.
>
> And when I read commit 073862ba5d24 ("netns: fix net_alloc_generic()"), it
> presented one possible issue with my first solution: in case a net_init
> call triggers access to a net ptr that has not been allocated, it may cause
> an issue. Thought I noticed later fixes in caif that may be related to
> this: it should not be possible to a subsystem to try to access its net ptr
> if it has not been initialized yet. And ops_init will only be called when
> there is enough room in struct net_generic, that is, net_assign_generic has
> been called.
>
> The only problem is that I cannot easily test that this fixes the issue. My
> tests for the first version involved adding a delay between the two reads
> of max_gen_ptrs and checking they were the same while forcing its
> increment.
>
> This has been observed in the field, though, with a KASAN splat:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in ops_init (/mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:0 /mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:129)
> Write of size 8 at addr ffff888131bd25b8 by task imageloader/4373
>
> CPU: 0 PID: 4373 Comm: imageloader Tainted: G     U            5.15.148-lockdep-21779-gb0a9bfb0a013 #1 db9ffbffbb2de989c984242ceea60881c9a62dd6
> Hardware name: Google Uldren/Uldren, BIOS Google_Uldren.15217.439.0 01/08/2024
> Call Trace:
> <TASK>
> dump_stack_lvl (/mnt/host/source/src/third_party/kernel/v5.15/lib/dump_stack.c:107 (discriminator 2))
> print_address_description (/mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/report.c:240 (discriminator 6))
> kasan_report (/mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/report.c:426 (discriminator 6) /mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/report.c:442)
> ops_init (/mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:0 /mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:129)
> setup_net (/mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:329)
> copy_net_ns (/mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:473)
> create_new_namespaces (/mnt/host/source/src/third_party/kernel/v5.15/kernel/nsproxy.c:110)
> unshare_nsproxy_namespaces (/mnt/host/source/src/third_party/kernel/v5.15/kernel/nsproxy.c:226 (discriminator 2))
> ksys_unshare (/mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3116)
> __x64_sys_unshare (/mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3190 /mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3188 /mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3188)
> do_syscall_64 (/mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/common.c:55 /mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/common.c:93)
> entry_SYSCALL_64_after_hwframe (/mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/entry_64.S:118)
> RIP: 0033:0x7a7494514457
> Code: 73 01 c3 48 8b 0d c1 a9 0b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 10 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 91 a9 0b 00 f7 d8 64 89 01 48
> All code
> ========
>
> Code starting with the faulting instruction
> ===========================================
> RSP: 002b:00007fff243cde08 EFLAGS: 00000206 ORIG_RAX: 0000000000000110
> RAX: ffffffffffffffda RBX: 0000599532577fe0 RCX: 00007a7494514457
> RDX: 0000000000000000 RSI: 00007a7494a0f38d RDI: 0000000040000000
> RBP: 00007fff243cdea0 R08: 0000000000000000 R09: 0000599532578a00
> R10: 0000000000044000 R11: 0000000000000206 R12: 00007fff243ce190
> R13: 00005995325748f0 R14: 0000000000000000 R15: 00007fff243ce221
> </TASK>
>
> Allocated by task 4373:
> stack_trace_save (/mnt/host/source/src/third_party/kernel/v5.15/kernel/stacktrace.c:123)
> kasan_save_stack (/mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/common.c:39)
> __kasan_kmalloc (/mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/common.c:46 /mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/common.c:434 /mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/common.c:513 /mnt/host/source/src/third_party/kernel/v5.15/mm/kasan/common.c:522)
> __kmalloc (/mnt/host/source/src/third_party/kernel/v5.15/include/linux/kasan.h:264 /mnt/host/source/src/third_party/kernel/v5.15/mm/slub.c:4407)
> copy_net_ns (/mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:75 /mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:401 /mnt/host/source/src/third_party/kernel/v5.15/net/core/net_namespace.c:460)
> create_new_namespaces (/mnt/host/source/src/third_party/kernel/v5.15/kernel/nsproxy.c:110)
> unshare_nsproxy_namespaces (/mnt/host/source/src/third_party/kernel/v5.15/kernel/nsproxy.c:226 (discriminator 2))
> ksys_unshare (/mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3116)
> __x64_sys_unshare (/mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3190 /mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3188 /mnt/host/source/src/third_party/kernel/v5.15/kernel/fork.c:3188)
> do_syscall_64 (/mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/common.c:55 /mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/common.c:93)
> entry_SYSCALL_64_after_hwframe (/mnt/host/source/src/third_party/kernel/v5.15/arch/x86/entry/entry_64.S:118)
>
> The buggy address belongs to the object at ffff888131bd2500
> which belongs to the cache kmalloc-192 of size 192
> The buggy address is located 184 bytes inside of
> 192-byte region [ffff888131bd2500, ffff888131bd25c0)
> The buggy address belongs to the page:
> page:000000009a3f4539 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x131bd2
> flags: 0x8000000000000200(slab|zone=2)
> raw: 8000000000000200 0000000000000000 dead000000000122 ffff888100043000
> raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888131bd2480: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> ffff888131bd2500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >ffff888131bd2580: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
> ^
> ffff888131bd2600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ffff888131bd2680: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
> ==================================================================
>
>
> From 32bb3d9ac830410cc5f8228580f2e2b9e6307069 Mon Sep 17 00:00:00 2001
> From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
> Date: Mon, 29 Apr 2024 11:56:44 -0300
> Subject: [PATCH] net: fix out-of-bounds access in ops_init
>
> net_alloc_generic is called by net_alloc, which is called without any
> locking. It reads max_gen_ptrs, which is changed under pernet_ops_rwsem. It
> is read twice, first to allocate an array, then to set s.len, which is
> later used to limit the bounds of the array access.
>
> It is possible that the array is allocated and another thread is
> registering a new pernet ops, increments max_gen_ptrs, which is then used
> to set s.len with a larger than allocated length for the variable array.
>
> Fix it by reading max_gen_ptrs only once in net_alloc_generic. If
> max_gen_ptrs is later incremented, it will be caught in net_assign_generic.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
> Fixes: 073862ba5d24 ("netns: fix net_alloc_generic()")
> ---
>  net/core/net_namespace.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index f0540c557515..4a4f0f87ee36 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -70,11 +70,13 @@ DEFINE_COOKIE(net_cookie);
>  static struct net_generic *net_alloc_generic(void)
>  {
>         struct net_generic *ng;
> -       unsigned int generic_size = offsetof(struct net_generic, ptr[max_gen_ptrs]);
> +       unsigned int generic_size;
> +       unsigned int gen_ptrs = READ_ONCE(max_gen_ptrs);
> +       generic_size = offsetof(struct net_generic, ptr[gen_ptrs]);
>
>         ng = kzalloc(generic_size, GFP_KERNEL);
>         if (ng)
> -               ng->s.len = max_gen_ptrs;
> +               ng->s.len = gen_ptrs;
>
>         return ng;
>  }
> @@ -1307,7 +1309,12 @@ static int register_pernet_operations(struct list_head *list,
>                 if (error < 0)
>                         return error;
>                 *ops->id = error;
> -               max_gen_ptrs = max(max_gen_ptrs, *ops->id + 1);
> +               /*
> +                * This does not require READ_ONCE as writers will take
> +                * pernet_ops_rwsem. But WRITE_ONCE is needed to protect
> +                * net_alloc_generic.
> +                */
> +               WRITE_ONCE(max_gen_ptrs, max(max_gen_ptrs, *ops->id + 1));
>         }
>         error = __register_pernet_operations(list, ops);
>         if (error) {
> --
> 2.34.1
>

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

I think you have to post this patch in the conventional way, so that
patchwork can catch up.

Thanks.

Powered by blists - more mailing lists