* [PATCH] net: fix out-of-bounds access in ops_init
@ 2024-04-30 8:42 Thadeu Lima de Souza Cascardo
2024-04-30 9:13 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2024-04-30 8:42 UTC (permalink / raw
To: netdev
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
kernel-dev, Thadeu Lima de Souza Cascardo
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@igalia.com>
---
net/core/net_namespace.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f0540c557515..879e49b10cca 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -87,7 +87,7 @@ static int net_assign_generic(struct net *net, unsigned int id, void *data)
old_ng = rcu_dereference_protected(net->gen,
lockdep_is_held(&pernet_ops_rwsem));
- if (old_ng->s.len > id) {
+ if (old_ng && old_ng->s.len > id) {
old_ng->ptr[id] = data;
return 0;
}
@@ -107,8 +107,9 @@ static int net_assign_generic(struct net *net, unsigned int id, void *data)
* the old copy for kfree after a grace period.
*/
- memcpy(&ng->ptr[MIN_PERNET_OPS_ID], &old_ng->ptr[MIN_PERNET_OPS_ID],
- (old_ng->s.len - MIN_PERNET_OPS_ID) * sizeof(void *));
+ if (old_ng)
+ memcpy(&ng->ptr[MIN_PERNET_OPS_ID], &old_ng->ptr[MIN_PERNET_OPS_ID],
+ (old_ng->s.len - MIN_PERNET_OPS_ID) * sizeof(void *));
ng->ptr[id] = data;
rcu_assign_pointer(net->gen, ng);
@@ -422,15 +423,10 @@ static struct workqueue_struct *netns_wq;
static struct net *net_alloc(void)
{
struct net *net = NULL;
- struct net_generic *ng;
-
- ng = net_alloc_generic();
- if (!ng)
- goto out;
net = kmem_cache_zalloc(net_cachep, GFP_KERNEL);
if (!net)
- goto out_free;
+ goto out;
#ifdef CONFIG_KEYS
net->key_domain = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
@@ -439,7 +435,7 @@ static struct net *net_alloc(void)
refcount_set(&net->key_domain->usage, 1);
#endif
- rcu_assign_pointer(net->gen, ng);
+ rcu_assign_pointer(net->gen, NULL);
out:
return net;
@@ -448,8 +444,6 @@ static struct net *net_alloc(void)
kmem_cache_free(net_cachep, net);
net = NULL;
#endif
-out_free:
- kfree(ng);
goto out;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: fix out-of-bounds access in ops_init
2024-04-30 8:42 [PATCH] net: fix out-of-bounds access in ops_init Thadeu Lima de Souza Cascardo
@ 2024-04-30 9:13 ` Eric Dumazet
2024-04-30 14:01 ` Thadeu Lima de Souza Cascardo
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2024-04-30 9:13 UTC (permalink / raw
To: Thadeu Lima de Souza Cascardo
Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni, kernel-dev
On Tue, Apr 30, 2024 at 10:43 AM Thadeu Lima de Souza Cascardo
<cascardo@igalia.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@igalia.com>
Good catch !
Could you provide a Fixes: tag ?
Have you considered reading max_gen_ptrs once in net_alloc_generic() ?
This would make the patch a little less complicated.
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) {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: fix out-of-bounds access in ops_init
2024-04-30 9:13 ` Eric Dumazet
@ 2024-04-30 14:01 ` Thadeu Lima de Souza Cascardo
2024-05-01 15:05 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2024-04-30 14:01 UTC (permalink / raw
To: Eric Dumazet
Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni, kernel-dev
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@igalia.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@igalia.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@igalia.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@igalia.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) {
>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: fix out-of-bounds access in ops_init
2024-04-30 14:01 ` Thadeu Lima de Souza Cascardo
@ 2024-05-01 15:05 ` Eric Dumazet
0 siblings, 0 replies; 4+ messages in thread
From: Eric Dumazet @ 2024-05-01 15:05 UTC (permalink / raw
To: Thadeu Lima de Souza Cascardo
Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni, kernel-dev
On Tue, Apr 30, 2024 at 4:01 PM Thadeu Lima de Souza Cascardo
<cascardo@igalia.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@igalia.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@igalia.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@igalia.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@igalia.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@google.com>
I think you have to post this patch in the conventional way, so that
patchwork can catch up.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-01 15:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 8:42 [PATCH] net: fix out-of-bounds access in ops_init Thadeu Lima de Souza Cascardo
2024-04-30 9:13 ` Eric Dumazet
2024-04-30 14:01 ` Thadeu Lima de Souza Cascardo
2024-05-01 15:05 ` Eric Dumazet
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 read-only IMAP folder(s) and NNTP newsgroup(s).