On Thu, Jul 14, 2016 at 10:15:34AM +0200, Jiri Slaby wrote: > From: Florian Westphal <fw@xxxxxxxxx> > > 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@xxxxxxxxx> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> > Signed-off-by: Jiri Slaby <jslaby@xxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html