Re: [PATCH 3.12 28/88] netfilter: x_tables: validate targets of jumps

[Jiri Slaby <jirislaby@gmail.com>]

On 07/14/2016, 10:15 AM, Jiri Slaby wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> 3.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ===============
> 
> commit 36472341017529e2b12573093cc0f68719300997 upstream.

I am now dropping this one. 3.12.62 will be released without that patch.
After the performance issue is resolved, it will be requeued.

> When we see a jump also check that the offset gets us to beginning of
> a rule (an ipt_entry).
> 
> The extra overhead is negible, even with absurd cases.
> 
> 300k custom rules, 300k jumps to 'next' user chain:
> [ plus one jump from INPUT to first userchain ]:
> 
> Before:
> real    0m24.874s
> user    0m7.532s
> sys     0m16.076s
> 
> After:
> real    0m27.464s
> user    0m7.436s
> sys     0m18.840s
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  net/ipv4/netfilter/arp_tables.c | 17 +++++++++++++++++
>  net/ipv4/netfilter/ip_tables.c  | 17 +++++++++++++++++
>  net/ipv6/netfilter/ip6_tables.c | 17 +++++++++++++++++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index 7460b7bef3ab..473ec559ce7b 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -358,6 +358,19 @@ 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 void *entry0,
> +			     const struct arpt_entry *target)
> +{
> +	struct arpt_entry *iter;
> +
> +	xt_entry_foreach(iter, entry0, 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.
>   */
> @@ -451,6 +464,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
>  					/* This a jump; chase it. */
>  					duprintf("Jump rule %u -> %u\n",
>  						 pos, newpos);
> +					e = (struct arpt_entry *)
> +						(entry0 + newpos);
> +					if (!find_jump_target(newinfo, entry0, e))
> +						return 0;
>  				} else {
>  					/* ... this is a fallthru */
>  					newpos = pos + e->next_offset;
> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> index 8fc22eed9603..dba9d8070d07 100644
> --- a/net/ipv4/netfilter/ip_tables.c
> +++ b/net/ipv4/netfilter/ip_tables.c
> @@ -434,6 +434,19 @@ ipt_do_table(struct sk_buff *skb,
>  #endif
>  }
>  
> +static bool find_jump_target(const struct xt_table_info *t,
> +			     const void *entry0,
> +			     const struct ipt_entry *target)
> +{
> +	struct ipt_entry *iter;
> +
> +	xt_entry_foreach(iter, entry0, 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
> @@ -531,6 +544,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
>  					/* This a jump; chase it. */
>  					duprintf("Jump rule %u -> %u\n",
>  						 pos, newpos);
> +					e = (struct ipt_entry *)
> +						(entry0 + newpos);
> +					if (!find_jump_target(newinfo, entry0, e))
> +						return 0;
>  				} else {
>  					/* ... this is a fallthru */
>  					newpos = pos + e->next_offset;
> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
> index 63f7876c4f29..97a8d2525c26 100644
> --- a/net/ipv6/netfilter/ip6_tables.c
> +++ b/net/ipv6/netfilter/ip6_tables.c
> @@ -444,6 +444,19 @@ ip6t_do_table(struct sk_buff *skb,
>  #endif
>  }
>  
> +static bool find_jump_target(const struct xt_table_info *t,
> +			     const void *entry0,
> +			     const struct ip6t_entry *target)
> +{
> +	struct ip6t_entry *iter;
> +
> +	xt_entry_foreach(iter, entry0, 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
> @@ -541,6 +554,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
>  					/* This a jump; chase it. */
>  					duprintf("Jump rule %u -> %u\n",
>  						 pos, newpos);
> +					e = (struct ip6t_entry *)
> +						(entry0 + newpos);
> +					if (!find_jump_target(newinfo, entry0, e))
> +						return 0;
>  				} else {
>  					/* ... this is a fallthru */
>  					newpos = pos + e->next_offset;
>