| Message ID | 044b51ecc8d0bc21faf8755e2f3014e8b65d71ec.1547569872.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: ip6_gre: use erspan key field for tunnel lookup | expand |
On Tue, 2019-01-15 at 17:43 +0100, Lorenzo Bianconi wrote: > Use ERSPAN key header field as tunnel key in gre_parse_header routine > since ERSPAN protocol sets the key field of the external GRE header to > 0 resulting in a possible tunnel lookup fail. > In addition remove key field parsing in erspan_rcv and ip6erspan_rcv > > Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support") > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > net/ipv4/gre_demux.c | 14 ++++++++++++++ > net/ipv4/ip_gre.c | 4 ---- > net/ipv6/ip6_gre.c | 1 - > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > index a4bf22ee3aed..ee09d657606e 100644 > --- a/net/ipv4/gre_demux.c > +++ b/net/ipv4/gre_demux.c > @@ -25,6 +25,7 @@ > #include <linux/spinlock.h> > #include <net/protocol.h> > #include <net/gre.h> > +#include <net/erspan.h> > > #include <net/icmp.h> > #include <net/route.h> > @@ -119,6 +120,19 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > hdr_len += 4; > } > tpi->hdr_len = hdr_len; > + > + /* ERSPAN ver 1 and 2 protocol sets GRE key field > + * to 0 and sets the configured key in the > + * inner erspan header field > + */ > + if (tpi->proto == htons(ETH_P_ERSPAN) || > + tpi->proto == htons(ETH_P_ERSPAN2)) { > + struct erspan_base_hdr *ershdr; > + > + ershdr = (struct erspan_base_hdr *)options; > + tpi->key = cpu_to_be32(get_session_id(ershdr)); > + } > + hi Lorenzo, are we sure that 'ershdr' is in the linear part of skb? if not, we might need a call to pskb_may_pull() here. thanks!
> > hi Lorenzo, > > are we sure that 'ershdr' is in the linear part of skb? if not, we might > need a call to pskb_may_pull() here. Hi Davide, thx for the review. I agree, I think we should check erspan header with pskb_may_pull in gre_parse_header. In this way we can remove the check in ip6erspan_rcv() and erspan_rcv(). I will fix in v2 Does '((*(u8 *)options & 0xF0) != 0x40)' have the same issue? Regards, Lorenzo > > thanks! > -- > davide > > >
On Wed, 2019-01-16 at 17:40 +0100, Lorenzo Bianconi wrote:
> Does '((*(u8 *)options & 0xF0) != 0x40)' have the same issue?
(cc-ing Jiri Benc, he probably knows more on this part)
in my opinion, yes, theoretically there might be situations where the
above line reads 1 byte outside the linear area of the skb. Probably the
fix for this is unrelated, and needs to be done in another patch.
For the record, git annotate on this line shows
00b203402984 ("gre: remove superfluous pskb_may_pull")
but I think it was reading out of the linear area also before that commit,
and that commit is correct, because pskb_may_pull() is called later using
the return value of gre_parse_header().
(other eyes over here are welcome :) )
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c index a4bf22ee3aed..ee09d657606e 100644 --- a/net/ipv4/gre_demux.c +++ b/net/ipv4/gre_demux.c @@ -25,6 +25,7 @@ #include <linux/spinlock.h> #include <net/protocol.h> #include <net/gre.h> +#include <net/erspan.h> #include <net/icmp.h> #include <net/route.h> @@ -119,6 +120,19 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, hdr_len += 4; } tpi->hdr_len = hdr_len; + + /* ERSPAN ver 1 and 2 protocol sets GRE key field + * to 0 and sets the configured key in the + * inner erspan header field + */ + if (tpi->proto == htons(ETH_P_ERSPAN) || + tpi->proto == htons(ETH_P_ERSPAN2)) { + struct erspan_base_hdr *ershdr; + + ershdr = (struct erspan_base_hdr *)options; + tpi->key = cpu_to_be32(get_session_id(ershdr)); + } + return hdr_len; } EXPORT_SYMBOL(gre_parse_header); diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index d1d09f3e5f9e..c9238ca67413 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -278,10 +278,6 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi, ershdr = (struct erspan_base_hdr *)(skb->data + gre_hdr_len); ver = ershdr->ver; - /* The original GRE header does not have key field, - * Use ERSPAN 10-bit session ID as key. - */ - tpi->key = cpu_to_be32(get_session_id(ershdr)); tunnel = ip_tunnel_lookup(itn, skb->dev->ifindex, tpi->flags | TUNNEL_KEY, iph->saddr, iph->daddr, tpi->key); diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 09d0826742f8..961a96355ddb 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -540,7 +540,6 @@ static int ip6erspan_rcv(struct sk_buff *skb, int gre_hdr_len, ipv6h = ipv6_hdr(skb); ershdr = (struct erspan_base_hdr *)skb->data; ver = ershdr->ver; - tpi->key = cpu_to_be32(get_session_id(ershdr)); tunnel = ip6gre_tunnel_lookup(skb->dev, &ipv6h->saddr, &ipv6h->daddr, tpi->key,
Use ERSPAN key header field as tunnel key in gre_parse_header routine since ERSPAN protocol sets the key field of the external GRE header to 0 resulting in a possible tunnel lookup fail. In addition remove key field parsing in erspan_rcv and ip6erspan_rcv Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support") Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- net/ipv4/gre_demux.c | 14 ++++++++++++++ net/ipv4/ip_gre.c | 4 ---- net/ipv6/ip6_gre.c | 1 - 3 files changed, 14 insertions(+), 5 deletions(-)