Skip to content

Commit

Permalink
netfilter: x_tables: speed up jump target validation
Browse files Browse the repository at this point in the history 
The dummy ruleset I used to test the original validation change was broken,
most rules were unreachable and were not tested by mark_source_chains().

In some cases rulesets that used to load in a few seconds now require
several minutes.

sample ruleset that shows the behaviour:

echo "*filter"
for i in $(seq 0 100000);do
        printf ":chain_%06x - [0:0]\n" $i
done
for i in $(seq 0 100000);do
   printf -- "-A INPUT -j chain_%06x\n" $i
   printf -- "-A INPUT -j chain_%06x\n" $i
   printf -- "-A INPUT -j chain_%06x\n" $i
done
echo COMMIT

[ pipe result into iptables-restore ]

This ruleset will be about 74mbyte in size, with ~500k searches
though all 500k[1] rule entries. iptables-restore will take forever
(gave up after 10 minutes)

Instead of always searching the entire blob for a match, fill an
array with the start offsets of every single ipt_entry struct,
then do a binary search to check if the jump target is present or not.

After this change ruleset restore times get again close to what one
gets when reverting 3647234 (~3 seconds on my workstation).

[1] every user-defined rule gets an implicit RETURN, so we get
300k jumps + 100k userchains + 100k returns -> 500k rule entries

Fixes: 3647234 ("netfilter: x_tables: validate targets of jumps")
Reported-by: Jeff Wu <wujiafu@gmail.com>
Tested-by: Jeff Wu <wujiafu@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
  • Loading branch information
Florian Westphal authored and Pablo Neira Ayuso committed Jul 18, 2016
1 parent 3101e0f commit f4dc777
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 64 deletions.
4 changes: 4 additions & 0 deletions include/linux/netfilter/x_tables.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ int xt_check_entry_offsets(const void *base, const char *elems,
unsigned int target_offset,
unsigned int next_offset);

unsigned int *xt_alloc_entry_offsets(unsigned int size);
bool xt_find_jump_offset(const unsigned int *offsets,
unsigned int target, unsigned int size);

int xt_check_match(struct xt_mtchk_param *, unsigned int size, u_int8_t proto,
bool inv_proto);
int xt_check_target(struct xt_tgchk_param *, unsigned int size, u_int8_t proto,
Expand Down
47 changes: 25 additions & 22 deletions net/ipv4/netfilter/arp_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,23 +299,12 @@ static inline bool unconditional(const struct arpt_entry *e)
memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
}

static bool find_jump_target(const struct xt_table_info *t,
const struct arpt_entry *target)
{
struct arpt_entry *iter;

xt_entry_foreach(iter, t->entries, t->size) {
if (iter == target)
return true;
}
return false;
}

/* Figures out from what hook each rule can be called: returns 0 if
* there are loops. Puts hook bitmask in comefrom.
*/
static int mark_source_chains(const struct xt_table_info *newinfo,
unsigned int valid_hooks, void *entry0)
unsigned int valid_hooks, void *entry0,
unsigned int *offsets)
{
unsigned int hook;

Expand Down Expand Up @@ -388,10 +377,11 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
/* This a jump; chase it. */
if (!xt_find_jump_offset(offsets, newpos,
newinfo->number))
return 0;
e = (struct arpt_entry *)
(entry0 + newpos);
if (!find_jump_target(newinfo, e))
return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
Expand Down Expand Up @@ -543,6 +533,7 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
const struct arpt_replace *repl)
{
struct arpt_entry *iter;
unsigned int *offsets;
unsigned int i;
int ret = 0;

Expand All @@ -555,6 +546,9 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
newinfo->underflow[i] = 0xFFFFFFFF;
}

offsets = xt_alloc_entry_offsets(newinfo->number);
if (!offsets)
return -ENOMEM;
i = 0;

/* Walk through entries, checking offsets. */
Expand All @@ -565,31 +559,37 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
repl->underflow,
repl->valid_hooks);
if (ret != 0)
break;
goto out_free;
if (i < repl->num_entries)
offsets[i] = (void *)iter - entry0;
++i;
if (strcmp(arpt_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
++newinfo->stacksize;
}
if (ret != 0)
return ret;
goto out_free;

ret = -EINVAL;
if (i != repl->num_entries)
return -EINVAL;
goto out_free;

/* Check hooks all assigned */
for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
/* Only hooks which are valid */
if (!(repl->valid_hooks & (1 << i)))
continue;
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
if (newinfo->underflow[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
}

if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
return -ELOOP;
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
ret = -ELOOP;
goto out_free;
}
kvfree(offsets);

/* Finally, each sanity check must pass */
i = 0;
Expand All @@ -609,6 +609,9 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
return ret;
}

return ret;
out_free:
kvfree(offsets);
return ret;
}

Expand Down
45 changes: 24 additions & 21 deletions net/ipv4/netfilter/ip_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,23 +373,12 @@ ipt_do_table(struct sk_buff *skb,
else return verdict;
}

static bool find_jump_target(const struct xt_table_info *t,
const struct ipt_entry *target)
{
struct ipt_entry *iter;

xt_entry_foreach(iter, t->entries, t->size) {
if (iter == target)
return true;
}
return false;
}

/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
mark_source_chains(const struct xt_table_info *newinfo,
unsigned int valid_hooks, void *entry0)
unsigned int valid_hooks, void *entry0,
unsigned int *offsets)
{
unsigned int hook;

Expand Down Expand Up @@ -458,10 +447,11 @@ mark_source_chains(const struct xt_table_info *newinfo,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
/* This a jump; chase it. */
if (!xt_find_jump_offset(offsets, newpos,
newinfo->number))
return 0;
e = (struct ipt_entry *)
(entry0 + newpos);
if (!find_jump_target(newinfo, e))
return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
Expand Down Expand Up @@ -694,6 +684,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
const struct ipt_replace *repl)
{
struct ipt_entry *iter;
unsigned int *offsets;
unsigned int i;
int ret = 0;

Expand All @@ -706,6 +697,9 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
newinfo->underflow[i] = 0xFFFFFFFF;
}

offsets = xt_alloc_entry_offsets(newinfo->number);
if (!offsets)
return -ENOMEM;
i = 0;
/* Walk through entries, checking offsets. */
xt_entry_foreach(iter, entry0, newinfo->size) {
Expand All @@ -715,29 +709,35 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
repl->underflow,
repl->valid_hooks);
if (ret != 0)
return ret;
goto out_free;
if (i < repl->num_entries)
offsets[i] = (void *)iter - entry0;
++i;
if (strcmp(ipt_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
++newinfo->stacksize;
}

ret = -EINVAL;
if (i != repl->num_entries)
return -EINVAL;
goto out_free;

/* Check hooks all assigned */
for (i = 0; i < NF_INET_NUMHOOKS; i++) {
/* Only hooks which are valid */
if (!(repl->valid_hooks & (1 << i)))
continue;
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
if (newinfo->underflow[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
}

if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
return -ELOOP;
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
ret = -ELOOP;
goto out_free;
}
kvfree(offsets);

/* Finally, each sanity check must pass */
i = 0;
Expand All @@ -757,6 +757,9 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
return ret;
}

return ret;
out_free:
kvfree(offsets);
return ret;
}

Expand Down
45 changes: 24 additions & 21 deletions net/ipv6/netfilter/ip6_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,23 +402,12 @@ ip6t_do_table(struct sk_buff *skb,
else return verdict;
}

static bool find_jump_target(const struct xt_table_info *t,
const struct ip6t_entry *target)
{
struct ip6t_entry *iter;

xt_entry_foreach(iter, t->entries, t->size) {
if (iter == target)
return true;
}
return false;
}

/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
mark_source_chains(const struct xt_table_info *newinfo,
unsigned int valid_hooks, void *entry0)
unsigned int valid_hooks, void *entry0,
unsigned int *offsets)
{
unsigned int hook;

Expand Down Expand Up @@ -487,10 +476,11 @@ mark_source_chains(const struct xt_table_info *newinfo,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
/* This a jump; chase it. */
if (!xt_find_jump_offset(offsets, newpos,
newinfo->number))
return 0;
e = (struct ip6t_entry *)
(entry0 + newpos);
if (!find_jump_target(newinfo, e))
return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
Expand Down Expand Up @@ -724,6 +714,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
const struct ip6t_replace *repl)
{
struct ip6t_entry *iter;
unsigned int *offsets;
unsigned int i;
int ret = 0;

Expand All @@ -736,6 +727,9 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
newinfo->underflow[i] = 0xFFFFFFFF;
}

offsets = xt_alloc_entry_offsets(newinfo->number);
if (!offsets)
return -ENOMEM;
i = 0;
/* Walk through entries, checking offsets. */
xt_entry_foreach(iter, entry0, newinfo->size) {
Expand All @@ -745,29 +739,35 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
repl->underflow,
repl->valid_hooks);
if (ret != 0)
return ret;
goto out_free;
if (i < repl->num_entries)
offsets[i] = (void *)iter - entry0;
++i;
if (strcmp(ip6t_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
++newinfo->stacksize;
}

ret = -EINVAL;
if (i != repl->num_entries)
return -EINVAL;
goto out_free;

/* Check hooks all assigned */
for (i = 0; i < NF_INET_NUMHOOKS; i++) {
/* Only hooks which are valid */
if (!(repl->valid_hooks & (1 << i)))
continue;
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
if (newinfo->underflow[i] == 0xFFFFFFFF)
return -EINVAL;
goto out_free;
}

if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
return -ELOOP;
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
ret = -ELOOP;
goto out_free;
}
kvfree(offsets);

/* Finally, each sanity check must pass */
i = 0;
Expand All @@ -787,6 +787,9 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
return ret;
}

return ret;
out_free:
kvfree(offsets);
return ret;
}

Expand Down
Loading

0 comments on commit f4dc777

Please sign in to comment.