netdev - Re: [PATCH v2] net: fix out-of-bounds access in ops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240501182819.34167-1-kuniyu@amazon.com>
Date: Wed, 1 May 2024 11:28:19 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <cascardo@...lia.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kernel-dev@...lia.com>,
	<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<kuniyu@...zon.com>
Subject: Re: [PATCH v2] net: fix out-of-bounds access in ops_init

From: Thadeu Lima de Souza Cascardo <cascardo@...lia.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@...lia.com>
> Fixes: 073862ba5d24 ("netns: fix net_alloc_generic()")
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.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