[Devel] [PATCH rh7] nf_conntrack: fix possible use-after-free on nf_conntrack module reloading.

[Vasily Averin]

Andrey,
could you please take look at vz6,
it seems it should be affected too, it isn't?

On 2018-06-18 14:54, Andrey Ryabinin wrote:
> Reloading nf_conntrack module with doubled hashsize parameter, i.e.
> 	  rmmod nf_conntrack
> 	  modprobe nf_conntrack hashsize=12345 hashsize=12345
> causes use-after-free.
> 
> On module unload, nf_conntrack_cleanup_net_list() frees init_net.ct.hash.
> On the next module loading nf_conntrack_set_hashsize() called twice,
> because hashsize parameter specified twice.
> On the first nf_conntrack_set_hashsize() call we just set
> 'nf_conntrack_htable_size' variable:
> 
> 	nf_conntrack_set_hashsize()
> 		...
> 		/* On boot, we can set this without any fancy locking. */
> 		if (!nf_conntrack_htable_size)
> 			return param_set_uint(val, kp);
> 
> But on the second invocation, nf_conntrack_htable_size is already set,
> so we pass further, use already freed init_net.ct.hash hashtable and
> free it again at the end:
> 	...
> 	old_size = init_net.ct.htable_size;
> 	old_hash = init_net.ct.hash;
> 
> 	init_net.ct.htable_size = nf_conntrack_htable_size = hashsize;
> 	init_net.ct.hash = hash;
> 
> 	....
> 	nf_ct_free_hashtable(old_hash, old_size);
> 
> Fix this, by checking !init_net.ct.htable_size instead of
> !nf_conntrack_htable_size. init_net.ct.htable_size will be initialized
> only in init callback, so on the second invocation of the
> nf_conntrack_set_hashsize() will just reinitialize nf_conntrack_htable_size
> and nothing more.
> Also set to zero init_net.ct on module removal, since init_net.ct.htable_size
> should be zero on next module loading.
> 
> https://pmc.acronis.com/browse/VSTOR-11099
> Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
> ---
>  net/netfilter/nf_conntrack_core.c       | 2 +-
>  net/netfilter/nf_conntrack_standalone.c | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 2337c1774a5d..8b3cec16ceb3 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1629,7 +1629,7 @@ int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp)
>  		return -EOPNOTSUPP;
>  
>  	/* On boot, we can set this without any fancy locking. */
> -	if (!nf_conntrack_htable_size)
> +	if (!init_net.ct.htable_size)
>  		return param_set_uint(val, kp);
>  
>  	rc = kstrtouint(val, 0, &hashsize);
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index aa23d68d146f..86fea808ade3 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -693,6 +693,12 @@ static void __exit nf_conntrack_standalone_fini(void)
>  {
>  	nf_conntrack_cleanup_start();
>  	unregister_pernet_subsys(&nf_conntrack_net_ops);
> +
> +	/*
> +	 * Make sure on the next module load we will not use dangling
> +	 * pointers.
> +	 */
> +	memset(&init_net.ct, 0, sizeof(init_net.ct));
>  	nf_conntrack_cleanup_end();
>  }
>  
>