Skip to content

Commit f4dc777

Browse files
Florian Westphalummakynes
Florian Westphal
authored and
ummakynes
committed
netfilter: x_tables: speed up jump target validation
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 <[email protected]> Tested-by: Jeff Wu <[email protected]> Signed-off-by: Florian Westphal <[email protected]> Signed-off-by: Pablo Neira Ayuso <[email protected]>
1 parent 3101e0f commit f4dc777

File tree

5 files changed

+127
-64
lines changed

5 files changed

+127
-64
lines changed

‎include/linux/netfilter/x_tables.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,10 @@ int xt_check_entry_offsets(const void *base, const char *elems,
250250
unsigned int target_offset,
251251
unsigned int next_offset);
252252

253+
unsigned int *xt_alloc_entry_offsets(unsigned int size);
254+
bool xt_find_jump_offset(const unsigned int *offsets,
255+
unsigned int target, unsigned int size);
256+
253257
int xt_check_match(struct xt_mtchk_param *, unsigned int size, u_int8_t proto,
254258
bool inv_proto);
255259
int xt_check_target(struct xt_tgchk_param *, unsigned int size, u_int8_t proto,

‎net/ipv4/netfilter/arp_tables.c

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -299,23 +299,12 @@ static inline bool unconditional(const struct arpt_entry *e)
299299
memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
300300
}
301301

302-
static bool find_jump_target(const struct xt_table_info *t,
303-
const struct arpt_entry *target)
304-
{
305-
struct arpt_entry *iter;
306-
307-
xt_entry_foreach(iter, t->entries, t->size) {
308-
if (iter == target)
309-
return true;
310-
}
311-
return false;
312-
}
313-
314302
/* Figures out from what hook each rule can be called: returns 0 if
315303
* there are loops. Puts hook bitmask in comefrom.
316304
*/
317305
static int mark_source_chains(const struct xt_table_info *newinfo,
318-
unsigned int valid_hooks, void *entry0)
306+
unsigned int valid_hooks, void *entry0,
307+
unsigned int *offsets)
319308
{
320309
unsigned int hook;
321310

@@ -388,10 +377,11 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
388377
XT_STANDARD_TARGET) == 0 &&
389378
newpos >= 0) {
390379
/* This a jump; chase it. */
380+
if (!xt_find_jump_offset(offsets, newpos,
381+
newinfo->number))
382+
return 0;
391383
e = (struct arpt_entry *)
392384
(entry0 + newpos);
393-
if (!find_jump_target(newinfo, e))
394-
return 0;
395385
} else {
396386
/* ... this is a fallthru */
397387
newpos = pos + e->next_offset;
@@ -543,6 +533,7 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
543533
const struct arpt_replace *repl)
544534
{
545535
struct arpt_entry *iter;
536+
unsigned int *offsets;
546537
unsigned int i;
547538
int ret = 0;
548539

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

549+
offsets = xt_alloc_entry_offsets(newinfo->number);
550+
if (!offsets)
551+
return -ENOMEM;
558552
i = 0;
559553

560554
/* Walk through entries, checking offsets. */
@@ -565,31 +559,37 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
565559
repl->underflow,
566560
repl->valid_hooks);
567561
if (ret != 0)
568-
break;
562+
goto out_free;
563+
if (i < repl->num_entries)
564+
offsets[i] = (void *)iter - entry0;
569565
++i;
570566
if (strcmp(arpt_get_target(iter)->u.user.name,
571567
XT_ERROR_TARGET) == 0)
572568
++newinfo->stacksize;
573569
}
574570
if (ret != 0)
575-
return ret;
571+
goto out_free;
576572

573+
ret = -EINVAL;
577574
if (i != repl->num_entries)
578-
return -EINVAL;
575+
goto out_free;
579576

580577
/* Check hooks all assigned */
581578
for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
582579
/* Only hooks which are valid */
583580
if (!(repl->valid_hooks & (1 << i)))
584581
continue;
585582
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
586-
return -EINVAL;
583+
goto out_free;
587584
if (newinfo->underflow[i] == 0xFFFFFFFF)
588-
return -EINVAL;
585+
goto out_free;
589586
}
590587

591-
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
592-
return -ELOOP;
588+
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
589+
ret = -ELOOP;
590+
goto out_free;
591+
}
592+
kvfree(offsets);
593593

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

612+
return ret;
613+
out_free:
614+
kvfree(offsets);
612615
return ret;
613616
}
614617

‎net/ipv4/netfilter/ip_tables.c

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -373,23 +373,12 @@ ipt_do_table(struct sk_buff *skb,
373373
else return verdict;
374374
}
375375

376-
static bool find_jump_target(const struct xt_table_info *t,
377-
const struct ipt_entry *target)
378-
{
379-
struct ipt_entry *iter;
380-
381-
xt_entry_foreach(iter, t->entries, t->size) {
382-
if (iter == target)
383-
return true;
384-
}
385-
return false;
386-
}
387-
388376
/* Figures out from what hook each rule can be called: returns 0 if
389377
there are loops. Puts hook bitmask in comefrom. */
390378
static int
391379
mark_source_chains(const struct xt_table_info *newinfo,
392-
unsigned int valid_hooks, void *entry0)
380+
unsigned int valid_hooks, void *entry0,
381+
unsigned int *offsets)
393382
{
394383
unsigned int hook;
395384

@@ -458,10 +447,11 @@ mark_source_chains(const struct xt_table_info *newinfo,
458447
XT_STANDARD_TARGET) == 0 &&
459448
newpos >= 0) {
460449
/* This a jump; chase it. */
450+
if (!xt_find_jump_offset(offsets, newpos,
451+
newinfo->number))
452+
return 0;
461453
e = (struct ipt_entry *)
462454
(entry0 + newpos);
463-
if (!find_jump_target(newinfo, e))
464-
return 0;
465455
} else {
466456
/* ... this is a fallthru */
467457
newpos = pos + e->next_offset;
@@ -694,6 +684,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
694684
const struct ipt_replace *repl)
695685
{
696686
struct ipt_entry *iter;
687+
unsigned int *offsets;
697688
unsigned int i;
698689
int ret = 0;
699690

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

700+
offsets = xt_alloc_entry_offsets(newinfo->number);
701+
if (!offsets)
702+
return -ENOMEM;
709703
i = 0;
710704
/* Walk through entries, checking offsets. */
711705
xt_entry_foreach(iter, entry0, newinfo->size) {
@@ -715,29 +709,35 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
715709
repl->underflow,
716710
repl->valid_hooks);
717711
if (ret != 0)
718-
return ret;
712+
goto out_free;
713+
if (i < repl->num_entries)
714+
offsets[i] = (void *)iter - entry0;
719715
++i;
720716
if (strcmp(ipt_get_target(iter)->u.user.name,
721717
XT_ERROR_TARGET) == 0)
722718
++newinfo->stacksize;
723719
}
724720

721+
ret = -EINVAL;
725722
if (i != repl->num_entries)
726-
return -EINVAL;
723+
goto out_free;
727724

728725
/* Check hooks all assigned */
729726
for (i = 0; i < NF_INET_NUMHOOKS; i++) {
730727
/* Only hooks which are valid */
731728
if (!(repl->valid_hooks & (1 << i)))
732729
continue;
733730
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
734-
return -EINVAL;
731+
goto out_free;
735732
if (newinfo->underflow[i] == 0xFFFFFFFF)
736-
return -EINVAL;
733+
goto out_free;
737734
}
738735

739-
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
740-
return -ELOOP;
736+
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
737+
ret = -ELOOP;
738+
goto out_free;
739+
}
740+
kvfree(offsets);
741741

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

760+
return ret;
761+
out_free:
762+
kvfree(offsets);
760763
return ret;
761764
}
762765

‎net/ipv6/netfilter/ip6_tables.c

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -402,23 +402,12 @@ ip6t_do_table(struct sk_buff *skb,
402402
else return verdict;
403403
}
404404

405-
static bool find_jump_target(const struct xt_table_info *t,
406-
const struct ip6t_entry *target)
407-
{
408-
struct ip6t_entry *iter;
409-
410-
xt_entry_foreach(iter, t->entries, t->size) {
411-
if (iter == target)
412-
return true;
413-
}
414-
return false;
415-
}
416-
417405
/* Figures out from what hook each rule can be called: returns 0 if
418406
there are loops. Puts hook bitmask in comefrom. */
419407
static int
420408
mark_source_chains(const struct xt_table_info *newinfo,
421-
unsigned int valid_hooks, void *entry0)
409+
unsigned int valid_hooks, void *entry0,
410+
unsigned int *offsets)
422411
{
423412
unsigned int hook;
424413

@@ -487,10 +476,11 @@ mark_source_chains(const struct xt_table_info *newinfo,
487476
XT_STANDARD_TARGET) == 0 &&
488477
newpos >= 0) {
489478
/* This a jump; chase it. */
479+
if (!xt_find_jump_offset(offsets, newpos,
480+
newinfo->number))
481+
return 0;
490482
e = (struct ip6t_entry *)
491483
(entry0 + newpos);
492-
if (!find_jump_target(newinfo, e))
493-
return 0;
494484
} else {
495485
/* ... this is a fallthru */
496486
newpos = pos + e->next_offset;
@@ -724,6 +714,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
724714
const struct ip6t_replace *repl)
725715
{
726716
struct ip6t_entry *iter;
717+
unsigned int *offsets;
727718
unsigned int i;
728719
int ret = 0;
729720

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

730+
offsets = xt_alloc_entry_offsets(newinfo->number);
731+
if (!offsets)
732+
return -ENOMEM;
739733
i = 0;
740734
/* Walk through entries, checking offsets. */
741735
xt_entry_foreach(iter, entry0, newinfo->size) {
@@ -745,29 +739,35 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
745739
repl->underflow,
746740
repl->valid_hooks);
747741
if (ret != 0)
748-
return ret;
742+
goto out_free;
743+
if (i < repl->num_entries)
744+
offsets[i] = (void *)iter - entry0;
749745
++i;
750746
if (strcmp(ip6t_get_target(iter)->u.user.name,
751747
XT_ERROR_TARGET) == 0)
752748
++newinfo->stacksize;
753749
}
754750

751+
ret = -EINVAL;
755752
if (i != repl->num_entries)
756-
return -EINVAL;
753+
goto out_free;
757754

758755
/* Check hooks all assigned */
759756
for (i = 0; i < NF_INET_NUMHOOKS; i++) {
760757
/* Only hooks which are valid */
761758
if (!(repl->valid_hooks & (1 << i)))
762759
continue;
763760
if (newinfo->hook_entry[i] == 0xFFFFFFFF)
764-
return -EINVAL;
761+
goto out_free;
765762
if (newinfo->underflow[i] == 0xFFFFFFFF)
766-
return -EINVAL;
763+
goto out_free;
767764
}
768765

769-
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
770-
return -ELOOP;
766+
if (!mark_source_chains(newinfo, repl->valid_hooks, entry0, offsets)) {
767+
ret = -ELOOP;
768+
goto out_free;
769+
}
770+
kvfree(offsets);
771771

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

790+
return ret;
791+
out_free:
792+
kvfree(offsets);
790793
return ret;
791794
}
792795

0 commit comments

Comments
 (0)