Skip to content

Commit

Permalink
netfilter: x_tables: do compat validation via translate_table
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Signed-off-by: Pablo Neira Ayuso <[email protected]>
  • Loading branch information
@ummakynes
Florian Westphal authored and ummakynes committed Apr 13, 2016
1 parent 0188346 commit 09d9686
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 342 deletions.
114 changes: 23 additions & 91 deletions net/ipv4/netfilter/arp_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1234,19 +1234,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 ||
Expand Down Expand Up @@ -1291,17 +1289,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:
Expand Down Expand Up @@ -1351,7 +1338,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;

Expand All @@ -1360,12 +1347,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);
Expand All @@ -1374,9 +1355,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;
Expand All @@ -1389,23 +1368,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)
Expand All @@ -1422,73 +1384,43 @@ 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;

i = 0;
xt_entry_foreach(iter1, entry1, newinfo->size) {
iter1->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(iter1->counters.pcnt)) {
ret = -ENOMEM;
break;
}
memcpy(&repl, compatr, sizeof(*compatr));

ret = check_target(iter1, compatr->name);
if (ret != 0) {
xt_percpu_counter_free(iter1->counters.pcnt);
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];
}

repl.num_counters = 0;
repl.counters = NULL;
repl.size = newinfo->size;
ret = translate_table(newinfo, entry1, &repl);
if (ret)
goto free_newinfo;

*pinfo = newinfo;
*pentry0 = entry1;
xt_free_table_info(info);
return 0;

free_newinfo:
xt_free_table_info(newinfo);
out:
return ret;
out_unlock:
xt_compat_flush_offsets(NFPROTO_ARP);
xt_compat_unlock(NFPROTO_ARP);
xt_entry_foreach(iter0, entry0, compatr->size) {
if (j-- == 0)
break;
compat_release_entry(iter0);
}
return ret;
out_unlock:
xt_compat_flush_offsets(NFPROTO_ARP);
xt_compat_unlock(NFPROTO_ARP);
goto out;
}

static int compat_do_replace(struct net *net, void __user *user,
Expand Down
Loading

0 comments on commit 09d9686

Please sign in to comment.