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

Submitted by Andrey Ryabinin on June 18, 2018, 11:54 a.m.

Details

Message ID 20180618115440.5570-1-aryabinin@virtuozzo.com
State New
Series "nf_conntrack: fix possible use-after-free on nf_conntrack module reloading."
Headers show

Commit Message

Andrey Ryabinin June 18, 2018, 11:54 a.m.
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@virtuozzo.com>
---
 net/netfilter/nf_conntrack_core.c       | 2 +-
 net/netfilter/nf_conntrack_standalone.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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();
 }
 

Comments

Vasily Averin June 18, 2018, 2:54 p.m.
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@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();
>  }
>  
>
Andrey Ryabinin June 18, 2018, 2:57 p.m.
On 06/18/2018 05:54 PM, Vasily Averin wrote:
> Andrey,
> could you please take look at vz6,
> it seems it should be affected too, it isn't?
> 

Yes, vz6 is also affected.
Vasily Averin June 18, 2018, 2:58 p.m.
On 2018-06-18 17:57, Andrey Ryabinin wrote:
> On 06/18/2018 05:54 PM, Vasily Averin wrote:
>> Andrey,
>> could you please take look at vz6,
>> it seems it should be affected too, it isn't?
> 
> Yes, vz6 is also affected.

JFYI I've submitted
https://jira.sw.ru/browse/PSBM-85938