Date:	Tue, 19 Jul 2016 09:13:26 +0200
From:	Michal Kubecek <mkubecek@...e.cz>
To:	Jiri Slaby <jslaby@...e.cz>
Cc:	stable@...r.kernel.org, linux-kernel@...r.kernel.org,
	Florian Westphal <fw@...len.de>,
	Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH 3.12 42/88] netfilter: x_tables: do compat validation via
 translate_table

On Thu, Jul 14, 2016 at 10:15:34AM +0200, Jiri Slaby wrote:
> From: Florian Westphal <fw@...len.de>
> 
> 3.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ===============
> 
> commit 09d9686047dbbe1cf4faa558d3ecc4aae2046054 upstream.
> 
> This looks like refactoring, but its also a bug fix.
> 
> Problem is that the compat path (32bit iptables, 64bit kernel) lacks a few
> sanity tests that are done in the normal path.
> 
> For example, we do not check for underflows and the base chain policies.
> 
> While its possible to also add such checks to the compat path, its more
> copy&pastry, for instance we cannot reuse check_underflow() helper as
> e->target_offset differs in the compat case.
> 
> Other problem is that it makes auditing for validation errors harder; two
> places need to be checked and kept in sync.
> 
> At a high level 32 bit compat works like this:
> 1- initial pass over blob:
>    validate match/entry offsets, bounds checking
>    lookup all matches and targets
>    do bookkeeping wrt. size delta of 32/64bit structures
>    assign match/target.u.kernel pointer (points at kernel
>    implementation, needed to access ->compatsize etc.)
> 
> 2- allocate memory according to the total bookkeeping size to
>    contain the translated ruleset
> 
> 3- second pass over original blob:
>    for each entry, copy the 32bit representation to the newly allocated
>    memory.  This also does any special match translations (e.g.
>    adjust 32bit to 64bit longs, etc).
> 
> 4- check if ruleset is free of loops (chase all jumps)
> 
> 5-first pass over translated blob:
>    call the checkentry function of all matches and targets.
> 
> The alternative implemented by this patch is to drop steps 3&4 from the
> compat process, the translation is changed into an intermediate step
> rather than a full 1:1 translate_table replacement.
> 
> In the 2nd pass (step #3), change the 64bit ruleset back to a kernel
> representation, i.e. put() the kernel pointer and restore ->u.user.name .
> 
> This gets us a 64bit ruleset that is in the format generated by a 64bit
> iptables userspace -- we can then use translate_table() to get the
> 'native' sanity checks.
> 
> This has two drawbacks:
> 
> 1. we re-validate all the match and target entry structure sizes even
> though compat translation is supposed to never generate bogus offsets.
> 2. we put and then re-lookup each match and target.
> 
> THe upside is that we get all sanity tests and ruleset validations
> provided by the normal path and can remove some duplicated compat code.
> 
> iptables-restore time of autogenerated ruleset with 300k chains of form
> -A CHAIN0001 -m limit --limit 1/s -j CHAIN0002
> -A CHAIN0002 -m limit --limit 1/s -j CHAIN0003
> 
> shows no noticeable differences in restore times:
> old:   0m30.796s
> new:   0m31.521s
> 64bit: 0m25.674s
> 
> Signed-off-by: Florian Westphal <fw@...len.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> Signed-off-by: Jiri Slaby <jslaby@...e.cz>
> ---
>  net/ipv4/netfilter/arp_tables.c | 109 ++++++-----------------------
>  net/ipv4/netfilter/ip_tables.c  | 151 ++++++++--------------------------------
>  net/ipv6/netfilter/ip6_tables.c | 145 ++++++--------------------------------
>  net/netfilter/x_tables.c        |   8 +++
>  4 files changed, 80 insertions(+), 333 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index 020b0e97c206..b7f3b31a3cc3 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -1223,19 +1223,17 @@ static inline void compat_release_entry(struct compat_arpt_entry *e)
>  	module_put(t->u.kernel.target->me);
>  }
>  
> -static inline int
> +static int
>  check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
>  				  struct xt_table_info *newinfo,
>  				  unsigned int *size,
>  				  const unsigned char *base,
> -				  const unsigned char *limit,
> -				  const unsigned int *hook_entries,
> -				  const unsigned int *underflows)
> +				  const unsigned char *limit)
>  {
>  	struct xt_entry_target *t;
>  	struct xt_target *target;
>  	unsigned int entry_offset;
> -	int ret, off, h;
> +	int ret, off;
>  
>  	duprintf("check_compat_entry_size_and_hooks %p\n", e);
>  	if ((unsigned long)e % __alignof__(struct compat_arpt_entry) != 0 ||
> @@ -1280,17 +1278,6 @@ check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
>  	if (ret)
>  		goto release_target;
>  
> -	/* Check hooks & underflows */
> -	for (h = 0; h < NF_ARP_NUMHOOKS; h++) {
> -		if ((unsigned char *)e - base == hook_entries[h])
> -			newinfo->hook_entry[h] = hook_entries[h];
> -		if ((unsigned char *)e - base == underflows[h])
> -			newinfo->underflow[h] = underflows[h];
> -	}
> -
> -	/* Clear counters and comefrom */
> -	memset(&e->counters, 0, sizeof(e->counters));
> -	e->comefrom = 0;
>  	return 0;
>  
>  release_target:
> @@ -1340,7 +1327,7 @@ static int translate_compat_table(struct xt_table_info **pinfo,
>  	struct xt_table_info *newinfo, *info;
>  	void *pos, *entry0, *entry1;
>  	struct compat_arpt_entry *iter0;
> -	struct arpt_entry *iter1;
> +	struct arpt_replace repl;
>  	unsigned int size;
>  	int ret = 0;
>  
> @@ -1349,12 +1336,6 @@ static int translate_compat_table(struct xt_table_info **pinfo,
>  	size = compatr->size;
>  	info->number = compatr->num_entries;
>  
> -	/* Init all hooks to impossible value. */
> -	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
> -		info->hook_entry[i] = 0xFFFFFFFF;
> -		info->underflow[i] = 0xFFFFFFFF;
> -	}
> -
>  	duprintf("translate_compat_table: size %u\n", info->size);
>  	j = 0;
>  	xt_compat_lock(NFPROTO_ARP);
> @@ -1363,9 +1344,7 @@ static int translate_compat_table(struct xt_table_info **pinfo,
>  	xt_entry_foreach(iter0, entry0, compatr->size) {
>  		ret = check_compat_entry_size_and_hooks(iter0, info, &size,
>  							entry0,
> -							entry0 + compatr->size,
> -							compatr->hook_entry,
> -							compatr->underflow);
> +							entry0 + compatr->size);
>  		if (ret != 0)
>  			goto out_unlock;
>  		++j;
> @@ -1378,23 +1357,6 @@ static int translate_compat_table(struct xt_table_info **pinfo,
>  		goto out_unlock;
>  	}
>  
> -	/* Check hooks all assigned */
> -	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
> -		/* Only hooks which are valid */
> -		if (!(compatr->valid_hooks & (1 << i)))
> -			continue;
> -		if (info->hook_entry[i] == 0xFFFFFFFF) {
> -			duprintf("Invalid hook entry %u %u\n",
> -				 i, info->hook_entry[i]);
> -			goto out_unlock;
> -		}
> -		if (info->underflow[i] == 0xFFFFFFFF) {
> -			duprintf("Invalid underflow %u %u\n",
> -				 i, info->underflow[i]);
> -			goto out_unlock;
> -		}
> -	}
> -
>  	ret = -ENOMEM;
>  	newinfo = xt_alloc_table_info(size);
>  	if (!newinfo)
> @@ -1411,51 +1373,25 @@ static int translate_compat_table(struct xt_table_info **pinfo,
>  	xt_entry_foreach(iter0, entry0, compatr->size)
>  		compat_copy_entry_from_user(iter0, &pos, &size,
>  					    newinfo, entry1);
> +
> +	/* all module references in entry0 are now gone */
> +
>  	xt_compat_flush_offsets(NFPROTO_ARP);
>  	xt_compat_unlock(NFPROTO_ARP);
>  
> -	ret = -ELOOP;
> -	if (!mark_source_chains(newinfo, compatr->valid_hooks, entry1))
> -		goto free_newinfo;
> +	memcpy(&repl, compatr, sizeof(*compatr));
>  
> -	i = 0;
> -	xt_entry_foreach(iter1, entry1, newinfo->size) {
> -		ret = check_target(iter1, compatr->name);
> -		if (ret != 0)
> -			break;
> -		++i;
> -		if (strcmp(arpt_get_target(iter1)->u.user.name,
> -		    XT_ERROR_TARGET) == 0)
> -			++newinfo->stacksize;
> -	}
> -	if (ret) {
> -		/*
> -		 * The first i matches need cleanup_entry (calls ->destroy)
> -		 * because they had called ->check already. The other j-i
> -		 * entries need only release.
> -		 */
> -		int skip = i;
> -		j -= i;
> -		xt_entry_foreach(iter0, entry0, newinfo->size) {
> -			if (skip-- > 0)
> -				continue;
> -			if (j-- == 0)
> -				break;
> -			compat_release_entry(iter0);
> -		}
> -		xt_entry_foreach(iter1, entry1, newinfo->size) {
> -			if (i-- == 0)
> -				break;
> -			cleanup_entry(iter1);
> -		}
> -		xt_free_table_info(newinfo);
> -		return ret;
> +	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
> +		repl.hook_entry[i] = newinfo->hook_entry[i];
> +		repl.underflow[i] = newinfo->underflow[i];
>  	}
>  
> -	/* And one copy for every other CPU */
> -	for_each_possible_cpu(i)
> -		if (newinfo->entries[i] && newinfo->entries[i] != entry1)
> -			memcpy(newinfo->entries[i], entry1, newinfo->size);

These four lines should be preserved, IMHO, as 3.12 doesn't have commit
482cfc318559 ("netfilter: xtables: avoid percpu ruleset duplication")
(introduced in 4.2) which removed the need for per-cpu copies.

The same applies to the other two instances of translate_compat_table()
in net/ipv4/netfilter/ip_tables.c and net/ipv6/netfilter/ip6_tables.c

Florian, do you agree?

Michal Kubecek

Powered by blists - more mailing lists