| 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 | expand |
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
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
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
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; }
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(-)