netfilter: x_tables: speed up jump target validation
authorFlorian Westphal <fw@strlen.de>
Thu, 14 Jul 2016 15:51:26 +0000 (17:51 +0200)
committerPablo Neira Ayuso <pablo@netfilter.org>
Mon, 18 Jul 2016 19:35:23 +0000 (21:35 +0200)
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 36472341017529e (~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: 36472341017529e ("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>
include/linux/netfilter/x_tables.h
net/ipv4/netfilter/arp_tables.c
net/ipv4/netfilter/ip_tables.c
net/ipv6/netfilter/ip6_tables.c
net/netfilter/x_tables.c

index e94e81ab2b58ead4f7e9d559d3a8571e63095f08..2ad1a2b289b5a57d25a3835e97302dd0db774f90 100644 (file)
@@ -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,
index c8dd9e26b1857904058b405a413a4a0d150f755e..b31df597fd37e4e8fa41777d7883973a6c852254 100644 (file)
@@ -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;
 
@@ -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;
@@ -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;
 
@@ -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. */
@@ -565,17 +559,20 @@ 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++) {
@@ -583,13 +580,16 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
                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;
@@ -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;
 }
 
index f0df66f54ce67035eae50624ca7e4863c765a3c7..f993545a337342d2fbb912b7560cd1529c0b8145 100644 (file)
@@ -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;
 
@@ -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;
@@ -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;
 
@@ -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) {
@@ -715,15 +709,18 @@ 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++) {
@@ -731,13 +728,16 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
                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;
@@ -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;
 }
 
index 61ed95054efa74fceb4740a22d263bc1b193fe9d..552fac2f390a2747c3ca9821fbb06d52ccaf0d9e 100644 (file)
@@ -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;
 
@@ -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;
@@ -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;
 
@@ -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) {
@@ -745,15 +739,18 @@ 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++) {
@@ -761,13 +758,16 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
                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;
@@ -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;
 }
 
index fe0e2db632c7426f9fcb0341aa21fecc58e62bb2..e0aa7c1d0224154db4ef09c752b88ca5e52bc404 100644 (file)
@@ -702,6 +702,56 @@ int xt_check_entry_offsets(const void *base,
 }
 EXPORT_SYMBOL(xt_check_entry_offsets);
 
+/**
+ * xt_alloc_entry_offsets - allocate array to store rule head offsets
+ *
+ * @size: number of entries
+ *
+ * Return: NULL or kmalloc'd or vmalloc'd array
+ */
+unsigned int *xt_alloc_entry_offsets(unsigned int size)
+{
+       unsigned int *off;
+
+       off = kcalloc(size, sizeof(unsigned int), GFP_KERNEL | __GFP_NOWARN);
+
+       if (off)
+               return off;
+
+       if (size < (SIZE_MAX / sizeof(unsigned int)))
+               off = vmalloc(size * sizeof(unsigned int));
+
+       return off;
+}
+EXPORT_SYMBOL(xt_alloc_entry_offsets);
+
+/**
+ * xt_find_jump_offset - check if target is a valid jump offset
+ *
+ * @offsets: array containing all valid rule start offsets of a rule blob
+ * @target: the jump target to search for
+ * @size: entries in @offset
+ */
+bool xt_find_jump_offset(const unsigned int *offsets,
+                        unsigned int target, unsigned int size)
+{
+       int m, low = 0, hi = size;
+
+       while (hi > low) {
+               m = (low + hi) / 2u;
+
+               if (offsets[m] > target)
+                       hi = m;
+               else if (offsets[m] < target)
+                       low = m + 1;
+               else
+                       return true;
+       }
+
+       return false;
+}
+EXPORT_SYMBOL(xt_find_jump_offset);
+
 int xt_check_target(struct xt_tgchk_param *par,
                    unsigned int size, u_int8_t proto, bool inv_proto)
 {