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+FuTSce_Q=uyn9brCDmwijf5-zOp3G9QDqSAaU=PC7=oCxUPQ@mail.gmail.com>
Date:   Wed, 1 Dec 2021 10:22:38 -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.

> > > +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?
>
> This is an ICMP error message. So we have an IP packet, skb, which
> contains in the message body the IP packet which invoked the error. If
> we pass skb to seg6_get_srh() it will look in the received ICMP
> packet. But we actually want to find the SRH in the packet which
> invoked the error, the one which is in the message body. So the code
> makes a clone of the skb, and then updates the pointers so that it
> points to the invoking packet within the ICMP packet. Then we can use
> seg6_get_srh() on this inner packet, since it just looks like an
> ordinary IP packet.

Ah of course. I clearly did not appreciate the importance of that
skb_reset_network_header.

> > 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_invoking? That seems to be the ICMP terminology?

Sounds good, thanks.

> > > +       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?
>
> I copied the idea from get_srh(). It does:
>
> srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>
> So i'm just undoing it.
>
> > Also, what happens if the header was in a frags that was pulled by
> > pskb_may_pull in seg6_get_srh.
>
> Yes, i checked that. Because the skb has been cloned, if it needs to
> rearrange the packet because it goes over a fragment boundary,
> pskb_may_pull() will return false. And then we won't find the
> SRH.

Great. So the feature only works if the SRH is in the linear header.

Then if the packet is not shared, you can just temporarily reset the
network header and revert it after?

> Nothing bad happens, traceroute is till broken as before.  What
> is a typical fragment size?

The question here is not the size in frags[], but that of the linear
section. This is really device driver and mtu specific. For many
devices and 1500 B mtu, the entire packet in linear seems quite
likely.