[nf-next,1/2] ebtables: add support for matching ICMP type and code

Message ID 4f8fa78149e0921c8efdc1adc5e12686ffe7667f.1520150717.git.mschiffer@universe-factory.net
State Accepted
Delegated to: Pablo Neira
Headers show
Series
  • ebtables: add support for ICMP and IGMP type/code matching
Related show

Commit Message

Matthias Schiffer March 4, 2018, 8:28 a.m.
We already have ICMPv6 type/code matches. This adds support for IPv4 ICMP
matches in the same way.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 include/uapi/linux/netfilter_bridge/ebt_ip.h | 13 +++++++--
 net/bridge/netfilter/ebt_ip.c                | 43 +++++++++++++++++++++-------
 2 files changed, 43 insertions(+), 13 deletions(-)

Comments

Pablo Neira Ayuso March 11, 2018, 9:14 p.m. | #1
On Sun, Mar 04, 2018 at 09:28:53AM +0100, Matthias Schiffer wrote:
> We already have ICMPv6 type/code matches. This adds support for IPv4 ICMP
> matches in the same way.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>  include/uapi/linux/netfilter_bridge/ebt_ip.h | 13 +++++++--
>  net/bridge/netfilter/ebt_ip.c                | 43 +++++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter_bridge/ebt_ip.h b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> index 8e462fb1983f..4ed7fbb0a482 100644
> --- a/include/uapi/linux/netfilter_bridge/ebt_ip.h
> +++ b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> @@ -24,8 +24,9 @@
>  #define EBT_IP_PROTO 0x08
>  #define EBT_IP_SPORT 0x10
>  #define EBT_IP_DPORT 0x20
> +#define EBT_IP_ICMP 0x40
>  #define EBT_IP_MASK (EBT_IP_SOURCE | EBT_IP_DEST | EBT_IP_TOS | EBT_IP_PROTO |\
> - EBT_IP_SPORT | EBT_IP_DPORT )
> +		     EBT_IP_SPORT | EBT_IP_DPORT | EBT_IP_ICMP)
>  #define EBT_IP_MATCH "ip"
>  
>  /* the same values are used for the invflags */
> @@ -38,8 +39,14 @@ struct ebt_ip_info {
>  	__u8  protocol;
>  	__u8  bitmask;
>  	__u8  invflags;
> -	__u16 sport[2];
> -	__u16 dport[2];
> +	union {
> +		__u16 sport[2];
> +		__u8 icmp_type[2];
> +	};
> +	union {
> +		__u16 dport[2];
> +		__u8 icmp_code[2];
> +	};

This is part of uapi, we cannot update struct ebt_ip_info, this break
binary compatibility.

>  };
>  
>  #endif
> diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
> index 2b46c50abce0..8cb8f8395768 100644
> --- a/net/bridge/netfilter/ebt_ip.c
> +++ b/net/bridge/netfilter/ebt_ip.c

Please, place these new matching features into
net/bridge/netfilter/ebt_ip.c, please add then new ebt_xyz.c extension
instead.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthias Schiffer March 11, 2018, 10:04 p.m. | #2
On 03/11/2018 10:14 PM, Pablo Neira Ayuso wrote:
> On Sun, Mar 04, 2018 at 09:28:53AM +0100, Matthias Schiffer wrote:
>> We already have ICMPv6 type/code matches. This adds support for IPv4 ICMP
>> matches in the same way.
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
>>  include/uapi/linux/netfilter_bridge/ebt_ip.h | 13 +++++++--
>>  net/bridge/netfilter/ebt_ip.c                | 43 +++++++++++++++++++++-------
>>  2 files changed, 43 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/uapi/linux/netfilter_bridge/ebt_ip.h b/include/uapi/linux/netfilter_bridge/ebt_ip.h
>> index 8e462fb1983f..4ed7fbb0a482 100644
>> --- a/include/uapi/linux/netfilter_bridge/ebt_ip.h
>> +++ b/include/uapi/linux/netfilter_bridge/ebt_ip.h
>> @@ -24,8 +24,9 @@
>>  #define EBT_IP_PROTO 0x08
>>  #define EBT_IP_SPORT 0x10
>>  #define EBT_IP_DPORT 0x20
>> +#define EBT_IP_ICMP 0x40
>>  #define EBT_IP_MASK (EBT_IP_SOURCE | EBT_IP_DEST | EBT_IP_TOS | EBT_IP_PROTO |\
>> - EBT_IP_SPORT | EBT_IP_DPORT )
>> +		     EBT_IP_SPORT | EBT_IP_DPORT | EBT_IP_ICMP)
>>  #define EBT_IP_MATCH "ip"
>>  
>>  /* the same values are used for the invflags */
>> @@ -38,8 +39,14 @@ struct ebt_ip_info {
>>  	__u8  protocol;
>>  	__u8  bitmask;
>>  	__u8  invflags;
>> -	__u16 sport[2];
>> -	__u16 dport[2];
>> +	union {
>> +		__u16 sport[2];
>> +		__u8 icmp_type[2];
>> +	};
>> +	union {
>> +		__u16 dport[2];
>> +		__u8 icmp_code[2];
>> +	};
> 
> This is part of uapi, we cannot update struct ebt_ip_info, this break
> binary compatibility.

I carefully extended the struct in a way that does not change API or ABI,
in the same way the corresponding ICMPv6 matches were added in 6faee60a4e
"netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes".

> 
>>  };
>>  
>>  #endif
>> diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
>> index 2b46c50abce0..8cb8f8395768 100644
>> --- a/net/bridge/netfilter/ebt_ip.c
>> +++ b/net/bridge/netfilter/ebt_ip.c
> 
> Please, place these new matching features into
> net/bridge/netfilter/ebt_ip.c, please add then new ebt_xyz.c extension
> instead.

This would increase asymmetry between ebt_ip and ebt_ip6, rather than
giving ebt_ip the same features that ebt_ip6 already had. Is there any good
reason to make it a new extension, when there is no problem to extend
ebt_ip without changing UAPI/ABI?

Regards,
Matthias
Pablo Neira Ayuso March 11, 2018, 10:18 p.m. | #3
On Sun, Mar 11, 2018 at 11:04:22PM +0100, Matthias Schiffer wrote:
> On 03/11/2018 10:14 PM, Pablo Neira Ayuso wrote:
> > On Sun, Mar 04, 2018 at 09:28:53AM +0100, Matthias Schiffer wrote:
> >> We already have ICMPv6 type/code matches. This adds support for IPv4 ICMP
> >> matches in the same way.
> >>
> >> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> >> ---
> >>  include/uapi/linux/netfilter_bridge/ebt_ip.h | 13 +++++++--
> >>  net/bridge/netfilter/ebt_ip.c                | 43 +++++++++++++++++++++-------
> >>  2 files changed, 43 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/netfilter_bridge/ebt_ip.h b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> index 8e462fb1983f..4ed7fbb0a482 100644
> >> --- a/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> +++ b/include/uapi/linux/netfilter_bridge/ebt_ip.h
> >> @@ -24,8 +24,9 @@
> >>  #define EBT_IP_PROTO 0x08
> >>  #define EBT_IP_SPORT 0x10
> >>  #define EBT_IP_DPORT 0x20
> >> +#define EBT_IP_ICMP 0x40
> >>  #define EBT_IP_MASK (EBT_IP_SOURCE | EBT_IP_DEST | EBT_IP_TOS | EBT_IP_PROTO |\
> >> - EBT_IP_SPORT | EBT_IP_DPORT )
> >> +		     EBT_IP_SPORT | EBT_IP_DPORT | EBT_IP_ICMP)
> >>  #define EBT_IP_MATCH "ip"
> >>  
> >>  /* the same values are used for the invflags */
> >> @@ -38,8 +39,14 @@ struct ebt_ip_info {
> >>  	__u8  protocol;
> >>  	__u8  bitmask;
> >>  	__u8  invflags;
> >> -	__u16 sport[2];
> >> -	__u16 dport[2];
> >> +	union {
> >> +		__u16 sport[2];
> >> +		__u8 icmp_type[2];
> >> +	};
> >> +	union {
> >> +		__u16 dport[2];
> >> +		__u8 icmp_code[2];
> >> +	};
> > 
> > This is part of uapi, we cannot update struct ebt_ip_info, this break
> > binary compatibility.
> 
> I carefully extended the struct in a way that does not change API or ABI,
> in the same way the corresponding ICMPv6 matches were added in 6faee60a4e
> "netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes".

I see, thanks for explaining.

> >>  };
> >>  
> >>  #endif
> >> diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
> >> index 2b46c50abce0..8cb8f8395768 100644
> >> --- a/net/bridge/netfilter/ebt_ip.c
> >> +++ b/net/bridge/netfilter/ebt_ip.c
> > 
> > Please, place these new matching features into
> > net/bridge/netfilter/ebt_ip.c, please add then new ebt_xyz.c extension
> > instead.
> 
> This would increase asymmetry between ebt_ip and ebt_ip6, rather than
> giving ebt_ip the same features that ebt_ip6 already had.

By looking at 6faee60a4e, you're right indeed.

> Is there any good reason to make it a new extension, when there is
> no problem to extend ebt_ip without changing UAPI/ABI?

I would suggest you place the IGMP match in a separated extension, so
we don't keep stuffing more code into ebt_ip which is rather going to
be called by everyone from the hotpath. I would expect few people are
going to need this, so I'd rather like to see this in the periphery.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/uapi/linux/netfilter_bridge/ebt_ip.h b/include/uapi/linux/netfilter_bridge/ebt_ip.h
index 8e462fb1983f..4ed7fbb0a482 100644
--- a/include/uapi/linux/netfilter_bridge/ebt_ip.h
+++ b/include/uapi/linux/netfilter_bridge/ebt_ip.h
@@ -24,8 +24,9 @@ 
 #define EBT_IP_PROTO 0x08
 #define EBT_IP_SPORT 0x10
 #define EBT_IP_DPORT 0x20
+#define EBT_IP_ICMP 0x40
 #define EBT_IP_MASK (EBT_IP_SOURCE | EBT_IP_DEST | EBT_IP_TOS | EBT_IP_PROTO |\
- EBT_IP_SPORT | EBT_IP_DPORT )
+		     EBT_IP_SPORT | EBT_IP_DPORT | EBT_IP_ICMP)
 #define EBT_IP_MATCH "ip"
 
 /* the same values are used for the invflags */
@@ -38,8 +39,14 @@  struct ebt_ip_info {
 	__u8  protocol;
 	__u8  bitmask;
 	__u8  invflags;
-	__u16 sport[2];
-	__u16 dport[2];
+	union {
+		__u16 sport[2];
+		__u8 icmp_type[2];
+	};
+	union {
+		__u16 dport[2];
+		__u8 icmp_code[2];
+	};
 };
 
 #endif
diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
index 2b46c50abce0..8cb8f8395768 100644
--- a/net/bridge/netfilter/ebt_ip.c
+++ b/net/bridge/netfilter/ebt_ip.c
@@ -19,9 +19,15 @@ 
 #include <linux/netfilter_bridge/ebtables.h>
 #include <linux/netfilter_bridge/ebt_ip.h>
 
-struct tcpudphdr {
-	__be16 src;
-	__be16 dst;
+union pkthdr {
+	struct {
+		__be16 src;
+		__be16 dst;
+	} tcpudphdr;
+	struct {
+		u8 type;
+		u8 code;
+	} icmphdr;
 };
 
 static bool
@@ -30,8 +36,8 @@  ebt_ip_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	const struct ebt_ip_info *info = par->matchinfo;
 	const struct iphdr *ih;
 	struct iphdr _iph;
-	const struct tcpudphdr *pptr;
-	struct tcpudphdr _ports;
+	const union pkthdr *pptr;
+	union pkthdr _pkthdr;
 
 	ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
 	if (ih == NULL)
@@ -50,29 +56,38 @@  ebt_ip_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (info->bitmask & EBT_IP_PROTO) {
 		if (NF_INVF(info, EBT_IP_PROTO, info->protocol != ih->protocol))
 			return false;
-		if (!(info->bitmask & EBT_IP_DPORT) &&
-		    !(info->bitmask & EBT_IP_SPORT))
+		if (!(info->bitmask & (EBT_IP_DPORT | EBT_IP_SPORT |
+				       EBT_IP_ICMP)))
 			return true;
 		if (ntohs(ih->frag_off) & IP_OFFSET)
 			return false;
+
+		/* min icmp headersize is 4, so sizeof(_pkthdr) is ok. */
 		pptr = skb_header_pointer(skb, ih->ihl*4,
-					  sizeof(_ports), &_ports);
+					  sizeof(_pkthdr), &_pkthdr);
 		if (pptr == NULL)
 			return false;
 		if (info->bitmask & EBT_IP_DPORT) {
-			u32 dst = ntohs(pptr->dst);
+			u32 dst = ntohs(pptr->tcpudphdr.dst);
 			if (NF_INVF(info, EBT_IP_DPORT,
 				    dst < info->dport[0] ||
 				    dst > info->dport[1]))
 				return false;
 		}
 		if (info->bitmask & EBT_IP_SPORT) {
-			u32 src = ntohs(pptr->src);
+			u32 src = ntohs(pptr->tcpudphdr.src);
 			if (NF_INVF(info, EBT_IP_SPORT,
 				    src < info->sport[0] ||
 				    src > info->sport[1]))
 				return false;
 		}
+		if ((info->bitmask & EBT_IP_ICMP) &&
+		    NF_INVF(info, EBT_IP_ICMP,
+			    pptr->icmphdr.type < info->icmp_type[0] ||
+			    pptr->icmphdr.type > info->icmp_type[1] ||
+			    pptr->icmphdr.code < info->icmp_code[0] ||
+			    pptr->icmphdr.code > info->icmp_code[1]))
+			return false;
 	}
 	return true;
 }
@@ -101,6 +116,14 @@  static int ebt_ip_mt_check(const struct xt_mtchk_param *par)
 		return -EINVAL;
 	if (info->bitmask & EBT_IP_SPORT && info->sport[0] > info->sport[1])
 		return -EINVAL;
+	if (info->bitmask & EBT_IP_ICMP) {
+		if ((info->invflags & EBT_IP_PROTO) ||
+		    info->protocol != IPPROTO_ICMP)
+			return -EINVAL;
+		if (info->icmp_type[0] > info->icmp_type[1] ||
+		    info->icmp_code[0] > info->icmp_code[1])
+			return -EINVAL;
+	}
 	return 0;
 }