| Message ID | 20240501151639.3369988-1-cascardo@igalia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: fix out-of-bounds access in ops_init | expand |
From: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> Date: Wed, 1 May 2024 12:16:39 -0300 > 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()") > Reviewed-by: Eric Dumazet <edumazet@google.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> The functional change looks good. I added small comments. > --- > 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); Please keep reverse xmas order, https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs unsigned int gen_ptrs = READ_ONCE(max_gen_ptrs); unsigned int generic_size; struct net_generic *ng; and add newline here. > + 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. > + */ Please use netdev multi-line comment style. https://docs.kernel.org/process/maintainer-netdev.html#multi-line-comments /* This does not require READ_ONCE as writers already hold * pernet_ops_rwsem. But WRITE_ONCE is needed to protect * net_alloc_generic. */ Perhaps s/will take/already hold/ as pernet_ops_rwsem is already held here. Also, could you repost with the target tree in Subject like [PATCH net v3] so that patchwork can apply it on net.git and test it properly. https://patchwork.kernel.org/project/netdevbpf/patch/20240501151639.3369988-1-cascardo@igalia.com/ Thanks! > + 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 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) {