On Thu, Jul 14, 2016 at 10:15:34AM +0200, Jiri Slaby wrote:
> From: Florian Westphal <f...@strlen.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 <f...@strlen.de>
> Signed-off-by: Pablo Neira Ayuso <pa...@netfilter.org>
> Signed-off-by: Jiri Slaby <jsl...@suse.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
