Message-ID: <ZjD5qm3mxdY/iebH@quatroqueijos.cascardo.eti.br>
Date: Tue, 30 Apr 2024 11:01:14 -0300
From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
To: Eric Dumazet <edumazet@...gle.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 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


> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 2f5190aa2f15cec2e934ebee9c502fb426cf0d7d..dc198ce7e6aeabd8831be32f0a3b5bd1d0a77315
> 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -70,11 +70,12 @@ 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]);
> +       /* Paired with WRITE_ONCE() in register_pernet_operations() */
> +       unsigned int max = READ_ONCE(max_gen_ptrs);
> 
> -       ng = kzalloc(generic_size, GFP_KERNEL);
> +       ng = kzalloc(offsetof(struct net_generic, ptr[max]), GFP_KERNEL);
>         if (ng)
> -               ng->s.len = max_gen_ptrs;
> +               ng->s.len = max;
> 
>         return ng;
>  }
> @@ -1308,7 +1309,9 @@ 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);
> +               /* Paired with READ_ONCE() in net_alloc_generic() */
> +               WRITE_ONCE(max_gen_ptrs,
> +                          max(max_gen_ptrs, *ops->id + 1));
>         }
>         error = __register_pernet_operations(list, ops);
>         if (error) {
> 

Powered by blists - more mailing lists