* [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes
@ 2010-12-17 15:27 Florian Westphal
2010-12-17 15:59 ` Jan Engelhardt
2010-12-18 16:33 ` Bart De Schuymer
0 siblings, 2 replies; 5+ messages in thread
From: Florian Westphal @ 2010-12-17 15:27 UTC (permalink / raw)
To: netfilter-devel
To avoid adding a new match revision icmp type/code are stored
in the sport/dport area.
Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Holger Eitzenberger <holger@eitzenberger.org>
---
patch for ebtables userspace will follow soon.
include/linux/netfilter_bridge/ebt_ip6.h | 15 +++++++++--
net/bridge/netfilter/ebt_ip6.c | 40 ++++++++++++++++++++++-------
2 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/include/linux/netfilter_bridge/ebt_ip6.h b/include/linux/netfilter_bridge/ebt_ip6.h
index e5de987..22af18a 100644
--- a/include/linux/netfilter_bridge/ebt_ip6.h
+++ b/include/linux/netfilter_bridge/ebt_ip6.h
@@ -18,8 +18,11 @@
#define EBT_IP6_PROTO 0x08
#define EBT_IP6_SPORT 0x10
#define EBT_IP6_DPORT 0x20
+#define EBT_IP6_ICMP6 0x40
+
#define EBT_IP6_MASK (EBT_IP6_SOURCE | EBT_IP6_DEST | EBT_IP6_TCLASS |\
- EBT_IP6_PROTO | EBT_IP6_SPORT | EBT_IP6_DPORT)
+ EBT_IP6_PROTO | EBT_IP6_SPORT | EBT_IP6_DPORT | \
+ EBT_IP6_ICMP6)
#define EBT_IP6_MATCH "ip6"
/* the same values are used for the invflags */
@@ -32,8 +35,14 @@ struct ebt_ip6_info {
uint8_t protocol;
uint8_t bitmask;
uint8_t invflags;
- uint16_t sport[2];
- uint16_t dport[2];
+ union {
+ uint16_t sport[2];
+ uint8_t icmpv6_type[2];
+ };
+ union {
+ uint16_t dport[2];
+ uint8_t icmpv6_code[2];
+ };
};
#endif
diff --git a/net/bridge/netfilter/ebt_ip6.c b/net/bridge/netfilter/ebt_ip6.c
index 50a46af..e6e5bbe 100644
--- a/net/bridge/netfilter/ebt_ip6.c
+++ b/net/bridge/netfilter/ebt_ip6.c
@@ -22,9 +22,15 @@
#include <linux/netfilter_bridge/ebtables.h>
#include <linux/netfilter_bridge/ebt_ip6.h>
-struct tcpudphdr {
- __be16 src;
- __be16 dst;
+union pkthdr {
+ struct {
+ __be16 src;
+ __be16 dst;
+ } tcpudphdr;
+ struct {
+ u8 type;
+ u8 code;
+ } icmphdr;
};
static bool
@@ -33,8 +39,8 @@ ebt_ip6_mt(const struct sk_buff *skb, struct xt_action_param *par)
const struct ebt_ip6_info *info = par->matchinfo;
const struct ipv6hdr *ih6;
struct ipv6hdr _ip6h;
- const struct tcpudphdr *pptr;
- struct tcpudphdr _ports;
+ const union pkthdr *pptr;
+ union pkthdr _ports;
ih6 = skb_header_pointer(skb, 0, sizeof(_ip6h), &_ip6h);
if (ih6 == NULL)
@@ -56,26 +62,32 @@ ebt_ip6_mt(const struct sk_buff *skb, struct xt_action_param *par)
return false;
if (FWINV(info->protocol != nexthdr, EBT_IP6_PROTO))
return false;
- if (!(info->bitmask & EBT_IP6_DPORT) &&
- !(info->bitmask & EBT_IP6_SPORT))
+ if (!(info->bitmask & ( EBT_IP6_DPORT |
+ EBT_IP6_SPORT | EBT_IP6_ICMP6)))
return true;
pptr = skb_header_pointer(skb, offset_ph, sizeof(_ports),
&_ports);
if (pptr == NULL)
return false;
if (info->bitmask & EBT_IP6_DPORT) {
- u32 dst = ntohs(pptr->dst);
+ u32 dst = ntohs(pptr->tcpudphdr.dst);
if (FWINV(dst < info->dport[0] ||
dst > info->dport[1], EBT_IP6_DPORT))
return false;
}
if (info->bitmask & EBT_IP6_SPORT) {
- u32 src = ntohs(pptr->src);
+ u32 src = ntohs(pptr->tcpudphdr.src);
if (FWINV(src < info->sport[0] ||
src > info->sport[1], EBT_IP6_SPORT))
return false;
}
- return true;
+ if ((info->bitmask & EBT_IP6_ICMP6) &&
+ FWINV(pptr->icmphdr.type < info->icmpv6_type[0] ||
+ pptr->icmphdr.type > info->icmpv6_type[1] ||
+ pptr->icmphdr.code < info->icmpv6_code[0] ||
+ pptr->icmphdr.code > info->icmpv6_code[1],
+ EBT_IP6_ICMP6))
+ return false;
}
return true;
}
@@ -103,6 +115,14 @@ static int ebt_ip6_mt_check(const struct xt_mtchk_param *par)
return -EINVAL;
if (info->bitmask & EBT_IP6_SPORT && info->sport[0] > info->sport[1])
return -EINVAL;
+ if (info->bitmask & EBT_IP6_ICMP6) {
+ if ((info->invflags & EBT_IP6_PROTO ||
+ info->protocol != IPPROTO_ICMPV6))
+ return -EINVAL;
+ if (info->icmpv6_type[0] > info->icmpv6_type[1] ||
+ info->icmpv6_code[0] > info->icmpv6_code[1])
+ return -EINVAL;
+ }
return 0;
}
--
1.7.2.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes
2010-12-17 15:27 [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes Florian Westphal
@ 2010-12-17 15:59 ` Jan Engelhardt
2010-12-17 16:21 ` Florian Westphal
2010-12-18 16:33 ` Bart De Schuymer
1 sibling, 1 reply; 5+ messages in thread
From: Jan Engelhardt @ 2010-12-17 15:59 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Friday 2010-12-17 16:27, Florian Westphal wrote:
>
>+++ ebt_ip6.c
> if (info->bitmask & EBT_IP6_DPORT) {
>- u32 dst = ntohs(pptr->dst);
>+ u32 dst = ntohs(pptr->tcpudphdr.dst);
The actual field however is smaller, so to me, this looks itching
to be changed to
uint16_t dst = ntohs(pptr->tcpudphdr.dst);
> if (FWINV(dst < info->dport[0] ||
> dst > info->dport[1], EBT_IP6_DPORT))
> return false;
> }
> if (info->bitmask & EBT_IP6_SPORT) {
>- u32 src = ntohs(pptr->src);
>+ u32 src = ntohs(pptr->tcpudphdr.src);
And so here.
>@@ -103,6 +115,14 @@ static int ebt_ip6_mt_check(const struct xt_mtchk_param *par)
> return -EINVAL;
> if (info->bitmask & EBT_IP6_SPORT && info->sport[0] > info->sport[1])
> return -EINVAL;
>+ if (info->bitmask & EBT_IP6_ICMP6) {
>+ if ((info->invflags & EBT_IP6_PROTO ||
>+ info->protocol != IPPROTO_ICMPV6))
>+ return -EINVAL;
Your parenthesis are a little out of balance, it's the & and | that
are commonly guarded. That probably should have been:
if ((info->invflags & EBT_IP6_PROTO) ||
info->protocol != IPPROTO_ICMPV6)
/* check other places too */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes
2010-12-17 15:59 ` Jan Engelhardt
@ 2010-12-17 16:21 ` Florian Westphal
2010-12-17 17:03 ` Jan Engelhardt
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2010-12-17 16:21 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Florian Westphal, netfilter-devel
Jan Engelhardt <jengelh@medozas.de> wrote:
> On Friday 2010-12-17 16:27, Florian Westphal wrote:
> >
> >+++ ebt_ip6.c
> > if (info->bitmask & EBT_IP6_DPORT) {
> >- u32 dst = ntohs(pptr->dst);
> >+ u32 dst = ntohs(pptr->tcpudphdr.dst);
>
> The actual field however is smaller, so to me, this looks itching
> to be changed to
> uint16_t dst = ntohs(pptr->tcpudphdr.dst);
Indeed.
> >@@ -103,6 +115,14 @@ static int ebt_ip6_mt_check(const struct xt_mtchk_param *par)
> > return -EINVAL;
> > if (info->bitmask & EBT_IP6_SPORT && info->sport[0] > info->sport[1])
> > return -EINVAL;
> >+ if (info->bitmask & EBT_IP6_ICMP6) {
> >+ if ((info->invflags & EBT_IP6_PROTO ||
> >+ info->protocol != IPPROTO_ICMPV6))
> >+ return -EINVAL;
>
> Your parenthesis are a little out of balance, it's the & and | that
> are commonly guarded. That probably should have been:
>
> if ((info->invflags & EBT_IP6_PROTO) ||
> info->protocol != IPPROTO_ICMPV6)
Right.
> /* check other places too */
You are right,
If (foo & bar && baz)
is used in other places as well.
But I would prefer to keep cleanups and functional changes separated.
I will make the u16 and the () changes, however I'll wait a couple of
hours to give others time to speak up.
Thanks for reviewing.
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes
2010-12-17 16:21 ` Florian Westphal
@ 2010-12-17 17:03 ` Jan Engelhardt
0 siblings, 0 replies; 5+ messages in thread
From: Jan Engelhardt @ 2010-12-17 17:03 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Friday 2010-12-17 17:21, Florian Westphal wrote:
>
>You are right,
> If (foo & bar && baz)
>
>is used in other places as well.
In that case, it is obvious.
The odd thing is the precedence of == in C before & and |.
In that regard you would not need any parentheses in the
quoted part besides those required by the if keyword:
>>+ if ((info->invflags & EBT_IP6_PROTO ||
>>+ info->protocol != IPPROTO_ICMPV6))
>>+ return -EINVAL;
>
>if ((info->invflags & EBT_IP6_PROTO) || info->protocol !=
>IPPROTO_ICMPV6)
[I guess that tells me I am already autoparenthizing subconsciously
when I would not need to.]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes
2010-12-17 15:27 [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes Florian Westphal
2010-12-17 15:59 ` Jan Engelhardt
@ 2010-12-18 16:33 ` Bart De Schuymer
1 sibling, 0 replies; 5+ messages in thread
From: Bart De Schuymer @ 2010-12-18 16:33 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
Hello Florian,
Thanks for the patch. It looks good. I have 2 comments however.
1. The call to skb_header_pointer doesn't always use the real needed
size: in case of icmp6, this is not sizeof(_ports). Since the icmpv6
header is at least 4 bytes, I guess this doesn't matter. But it might be
good to add a comment.
2. The name _ports is no longer descriptive.
cheers,
Bart
On 17-12-10 16:27, Florian Westphal wrote:
> To avoid adding a new match revision icmp type/code are stored
> in the sport/dport area.
>
> Signed-off-by: Florian Westphal<fw@strlen.de>
> Reviewed-by: Holger Eitzenberger<holger@eitzenberger.org>
> ---
> patch for ebtables userspace will follow soon.
>
> include/linux/netfilter_bridge/ebt_ip6.h | 15 +++++++++--
> net/bridge/netfilter/ebt_ip6.c | 40 ++++++++++++++++++++++-------
> 2 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/netfilter_bridge/ebt_ip6.h b/include/linux/netfilter_bridge/ebt_ip6.h
> index e5de987..22af18a 100644
> --- a/include/linux/netfilter_bridge/ebt_ip6.h
> +++ b/include/linux/netfilter_bridge/ebt_ip6.h
> @@ -18,8 +18,11 @@
> #define EBT_IP6_PROTO 0x08
> #define EBT_IP6_SPORT 0x10
> #define EBT_IP6_DPORT 0x20
> +#define EBT_IP6_ICMP6 0x40
> +
> #define EBT_IP6_MASK (EBT_IP6_SOURCE | EBT_IP6_DEST | EBT_IP6_TCLASS |\
> - EBT_IP6_PROTO | EBT_IP6_SPORT | EBT_IP6_DPORT)
> + EBT_IP6_PROTO | EBT_IP6_SPORT | EBT_IP6_DPORT | \
> + EBT_IP6_ICMP6)
> #define EBT_IP6_MATCH "ip6"
>
> /* the same values are used for the invflags */
> @@ -32,8 +35,14 @@ struct ebt_ip6_info {
> uint8_t protocol;
> uint8_t bitmask;
> uint8_t invflags;
> - uint16_t sport[2];
> - uint16_t dport[2];
> + union {
> + uint16_t sport[2];
> + uint8_t icmpv6_type[2];
> + };
> + union {
> + uint16_t dport[2];
> + uint8_t icmpv6_code[2];
> + };
> };
>
> #endif
> diff --git a/net/bridge/netfilter/ebt_ip6.c b/net/bridge/netfilter/ebt_ip6.c
> index 50a46af..e6e5bbe 100644
> --- a/net/bridge/netfilter/ebt_ip6.c
> +++ b/net/bridge/netfilter/ebt_ip6.c
> @@ -22,9 +22,15 @@
> #include<linux/netfilter_bridge/ebtables.h>
> #include<linux/netfilter_bridge/ebt_ip6.h>
>
> -struct tcpudphdr {
> - __be16 src;
> - __be16 dst;
> +union pkthdr {
> + struct {
> + __be16 src;
> + __be16 dst;
> + } tcpudphdr;
> + struct {
> + u8 type;
> + u8 code;
> + } icmphdr;
> };
>
> static bool
> @@ -33,8 +39,8 @@ ebt_ip6_mt(const struct sk_buff *skb, struct xt_action_param *par)
> const struct ebt_ip6_info *info = par->matchinfo;
> const struct ipv6hdr *ih6;
> struct ipv6hdr _ip6h;
> - const struct tcpudphdr *pptr;
> - struct tcpudphdr _ports;
> + const union pkthdr *pptr;
> + union pkthdr _ports;
>
> ih6 = skb_header_pointer(skb, 0, sizeof(_ip6h),&_ip6h);
> if (ih6 == NULL)
> @@ -56,26 +62,32 @@ ebt_ip6_mt(const struct sk_buff *skb, struct xt_action_param *par)
> return false;
> if (FWINV(info->protocol != nexthdr, EBT_IP6_PROTO))
> return false;
> - if (!(info->bitmask& EBT_IP6_DPORT)&&
> - !(info->bitmask& EBT_IP6_SPORT))
> + if (!(info->bitmask& ( EBT_IP6_DPORT |
> + EBT_IP6_SPORT | EBT_IP6_ICMP6)))
> return true;
> pptr = skb_header_pointer(skb, offset_ph, sizeof(_ports),
> &_ports);
> if (pptr == NULL)
> return false;
> if (info->bitmask& EBT_IP6_DPORT) {
> - u32 dst = ntohs(pptr->dst);
> + u32 dst = ntohs(pptr->tcpudphdr.dst);
> if (FWINV(dst< info->dport[0] ||
> dst> info->dport[1], EBT_IP6_DPORT))
> return false;
> }
> if (info->bitmask& EBT_IP6_SPORT) {
> - u32 src = ntohs(pptr->src);
> + u32 src = ntohs(pptr->tcpudphdr.src);
> if (FWINV(src< info->sport[0] ||
> src> info->sport[1], EBT_IP6_SPORT))
> return false;
> }
> - return true;
> + if ((info->bitmask& EBT_IP6_ICMP6)&&
> + FWINV(pptr->icmphdr.type< info->icmpv6_type[0] ||
> + pptr->icmphdr.type> info->icmpv6_type[1] ||
> + pptr->icmphdr.code< info->icmpv6_code[0] ||
> + pptr->icmphdr.code> info->icmpv6_code[1],
> + EBT_IP6_ICMP6))
> + return false;
> }
> return true;
> }
> @@ -103,6 +115,14 @@ static int ebt_ip6_mt_check(const struct xt_mtchk_param *par)
> return -EINVAL;
> if (info->bitmask& EBT_IP6_SPORT&& info->sport[0]> info->sport[1])
> return -EINVAL;
> + if (info->bitmask& EBT_IP6_ICMP6) {
> + if ((info->invflags& EBT_IP6_PROTO ||
> + info->protocol != IPPROTO_ICMPV6))
> + return -EINVAL;
> + if (info->icmpv6_type[0]> info->icmpv6_type[1] ||
> + info->icmpv6_code[0]> info->icmpv6_code[1])
> + return -EINVAL;
> + }
> return 0;
> }
>
--
Bart De Schuymer
www.artinalgorithms.be
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-12-18 16:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-17 15:27 [PATCH] netfilter: ebt_ip6: allow matching on ipv6-icmp types/codes Florian Westphal
2010-12-17 15:59 ` Jan Engelhardt
2010-12-17 16:21 ` Florian Westphal
2010-12-17 17:03 ` Jan Engelhardt
2010-12-18 16:33 ` Bart De Schuymer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).