netdev - Re: [patch RFC net-next 2/3] icmp: ICMPV6: Examine invoking packet for Segment Route Headers.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTSfLxEic2ZtD8ygzUQMrftLkRyfjdf7GH6Pf8ioSRjHrOg@mail.gmail.com>
Date:   Wed, 1 Dec 2021 09:33:32 -0800
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        James Prestwood <prestwoj@...il.com>,
        Justin Iurman <justin.iurman@...ege.be>,
        Praveen Chaudhary <praveen5582@...il.com>,
        "Jason A . Donenfeld" <Jason@...c4.com>,
        Eric Dumazet <edumazet@...gle.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [patch RFC net-next 2/3] icmp: ICMPV6: Examine invoking packet
 for Segment Route Headers.

>  include/linux/ipv6.h |  2 ++
>  net/ipv6/icmp.c      | 36 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> index 20c1f968da7c..d8ab5022d397 100644
> --- a/include/linux/ipv6.h
> +++ b/include/linux/ipv6.h
> @@ -133,6 +133,7 @@ struct inet6_skb_parm {
>         __u16                   dsthao;
>  #endif
>         __u16                   frag_max_size;
> +       __u16                   srhoff;

Out of scope for this patch, but I guess we could use a

BUILD_BUG_ON(sizeof(struct inet6_skb_parm) > sizeof_field(struct sk_buff, cb));

>
>  #define IP6SKB_XFRM_TRANSFORMED        1
>  #define IP6SKB_FORWARDED       2
> @@ -142,6 +143,7 @@ struct inet6_skb_parm {
>  #define IP6SKB_HOPBYHOP        32
>  #define IP6SKB_L3SLAVE         64
>  #define IP6SKB_JUMBOGRAM      128
> +#define IP6SKB_SEG6          512

256?

>  };
>
>  #if defined(CONFIG_NET_L3_MASTER_DEV)
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index a7c31ab67c5d..315787b79f29 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -57,6 +57,7 @@
>  #include <net/protocol.h>
>  #include <net/raw.h>
>  #include <net/rawv6.h>
> +#include <net/seg6.h>
>  #include <net/transp_v6.h>
>  #include <net/ip6_route.h>
>  #include <net/addrconf.h>
> @@ -818,9 +819,40 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
>         local_bh_enable();
>  }
>
> +/* Determine if the invoking packet contains a segment routing header.
> + * If it does, extract the true destination address, which is the
> + * first segment address
> + */
> +static void icmpv6_notify_srh(struct sk_buff *skb, struct inet6_skb_parm *opt)
> +{
> +       struct sk_buff *skb_orig;
> +       struct ipv6_sr_hdr *srh;
> +
> +       skb_orig = skb_clone(skb, GFP_ATOMIC);
> +       if (!skb_orig)
> +               return;

Is this to be allowed to write to skb->cb? Or because seg6_get_srh
calls pskb_may_pull to parse the headers?

It is unlikely (not impossible) in this path for the packet to be
shared or cloned. Avoid this operation when it isn't? Most packets
will not actually have segment routing, so this imposes significant
cost on the common case (if in the not common ICMP processing path).

nit: I found the name skb_orig confusing, as it is not in the meaning
of preserve the original skb as at function entry.

> +       skb_dst_drop(skb_orig);
> +       skb_reset_network_header(skb_orig);
> +
> +       srh = seg6_get_srh(skb_orig, 0);
> +       if (!srh)
> +               goto out;
> +
> +       if (srh->type != IPV6_SRCRT_TYPE_4)
> +               goto out;
> +
> +       opt->flags |= IP6SKB_SEG6;
> +       opt->srhoff = (unsigned char *)srh - skb->data;

Should this offset be against skb->head, in case other data move
operations could occur?

Also, what happens if the header was in a frags that was pulled by
pskb_may_pull in seg6_get_srh.

If we can expect headers to exist in the linear segment, then perhaps
the whole code can be simplified and the clone can be avoided.

> +
> +out:
> +       kfree_skb(skb_orig);
> +}
> +
>  void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
>  {
>         const struct inet6_protocol *ipprot;
> +       struct inet6_skb_parm *opt = IP6CB(skb);
>         int inner_offset;
>         __be16 frag_off;
>         u8 nexthdr;
> @@ -829,6 +861,8 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
>         if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
>                 goto out;
>
> +       icmpv6_notify_srh(skb, opt);
> +
>         nexthdr = ((struct ipv6hdr *)skb->data)->nexthdr;
>         if (ipv6_ext_hdr(nexthdr)) {
>                 /* now skip over extension headers */
> @@ -853,7 +887,7 @@ void icmpv6_notify(struct sk_buff *skb, u8 type, u8 code, __be32 info)
>
>         ipprot = rcu_dereference(inet6_protos[nexthdr]);
>         if (ipprot && ipprot->err_handler)
> -               ipprot->err_handler(skb, NULL, type, code, inner_offset, info);
> +               ipprot->err_handler(skb, opt, type, code, inner_offset, info);
>
>         raw6_icmp_error(skb, nexthdr, type, code, inner_offset, info);
>         return;
> --
> 2.33.1
>