Date:   Sat, 4 Sep 2021 16:47:04 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ido Schimmel <idosch@...sch.org>,
        chouhan.shreyansh630@...il.com
Subject: Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM_PARTIAL

On Sat, Sep 4, 2021 at 3:05 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Sat, Sep 4, 2021 at 5:54 PM Alexander Duyck
> <alexander.duyck@...il.com> wrote:
> >
> > On Sat, Sep 4, 2021 at 2:40 PM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > On Sat, Sep 4, 2021 at 11:37 AM Alexander Duyck
> > > <alexander.duyck@...il.com> wrote:
> > > >
> > > > On Sat, Sep 4, 2021 at 7:46 AM Willem de Bruijn
> > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > >
> > > > > On Fri, Sep 3, 2021 at 7:27 PM Alexander Duyck
> > > > > <alexander.duyck@...il.com> wrote:
> > > > > >
> > > > > > On Fri, Sep 3, 2021 at 12:38 PM Willem de Bruijn
> > > > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > > > >
> > > > > >
> > > > > > <snip>
> > > > > >
> > > > > > > > whereas if the offset is stored somewhere in the unstripped data we
> > > > > > > > could then drop the packet and count it as a drop without having to
> > > > > > > > modify the frame via the skb_pull.
> > > > > > >
> > > > > > > This is a broader issue that userspace can pass any csum_start as long
> > > > > > > as it is within packet bounds. We could address it here specifically
> > > > > > > for the GRE header. But that still leaves many potentially bad offsets
> > > > > > > further in the packet in this case, and all the other cases. Checking
> > > > > > > that specific header seems a bit arbitrary to me, and might actually
> > > > > > > give false confidence.
> > > > > > >
> > > > > > > We could certainly move the validation from gre_handle_offloads to
> > > > > > > before skb_pull, to make it more obvious *why* the check exists.
> > > > > >
> > > > > > Agreed. My main concern is that the csum_start is able to be located
> > > > > > somewhere where the userspace didn't write. For the most part the
> > > > > > csum_start and csum_offset just needs to be restricted to the regions
> > > > > > that the userspace actually wrote to.
> > > > >
> > > > > I don't quite follow. Even with this bug, the offset is somewhere userspace
> > > > > wrote. That data is just pulled.
> > > >
> > > > Sorry, I was thinking of the SOCK_DGRAM case where the header is added
> > > > via a call to dev_hard_header().
> > > >
> > > > > > > > Maybe for those
> > > > > > > > cases we need to look at adding an unsigned int argument to
> > > > > > > > virtio_net_hdr_to_skb in which we could pass 0 for the unused case or
> > > > > > > > dev->hard_header_len in the cases where we have something like
> > > > > > > > af_packet that is transmitting over an ipgre tunnel. The general idea
> > > > > > > > is to prevent these virtio_net_hdr_to_skb calls from pointing the
> > > > > > > > csum_start into headers that userspace was not responsible for
> > > > > > > > populating.
> > > > > > >
> > > > > > > One issue with that is that dev->hard_header_len itself is imprecise
> > > > > > > for protocols with variable length link layer headers. There, too, we
> > > > > > > have had a variety of bug fixes in the past.
> > > > > > >
> > > > > > > It also adds cost to every user of virtio_net_hdr, while we only know
> > > > > > > one issue in a rare case of the IP_GRE device.
> > > > > >
> > > > > > Quick question, the assumption is that the checksum should always be
> > > > > > performed starting no earlier than the transport header right? Looking
> > > > > > over virtio_net_hdr_to_skb it looks like it is already verifying the
> > > > > > transport header is in the linear portion of the skb. I'm wondering if
> > > > > > we couldn't just look at adding a check to verify the transport offset
> > > > > > is <= csum start? We might also be able to get rid of one of the two
> > > > > > calls to pskb_may_pull by doing that.
> > > > >
> > > > > Are you referring to this part in the .._NEEDS_CSUM branch?
> > > > >
> > > > >                 if (!skb_partial_csum_set(skb, start, off))
> > > > >                         return -EINVAL;
> > > > >
> > > > >                 p_off = skb_transport_offset(skb) + thlen;
> > > > >                 if (!pskb_may_pull(skb, p_off))
> > > > >                         return -EINVAL;
> > > > >
> > > > > skb_partial_csum_set is actually what sets the transport offset,
> > > > > derived from start.
> > > >
> > > > Ugh, I had overlooked that as I was more focused on the
> > > > skb_probe_transport_header calls in the af_packet code.
> > > >
> > > > So we can have both the transport offset and the csum_start in a
> > > > region that gets stripped by the ipgre code. Worse yet the inner
> > > > transport header will also be pointing somewhere outside of the
> > > > encapsulated region when we pass it off to skb_reset_inner_headers().
> > > >
> > > > Maybe it would make sense to just have the check look into the
> > > > transport offset instead of csum start as that way you are essentially
> > > > addressing two possible issues instead of one, and it would
> > > > effectively combine multiple checks as the uninitialized value is ~0
> > > > which should always be greater than "skb_headroom + tunnel->hlen +
> > > > sizeof(struct iphdr)". I think you mentioned before placing a check
> > > > just before you make the call to skb_pull in the GRE transmit path.
> > > > Doing that we would at least reduce the impact as it would only apply
> > > > in the header_ops case in ipgre_xmit instead of being applied to all
> > > > the transmit paths which don't perform the pull.
> > >
> > > Do you mean
> > >
> > >         if (dev->header_ops) {
> > > +               int pull_len = tunnel->hlen + sizeof(struct iphdr);
> > > +
> > >                 if (skb_cow_head(skb, 0))
> > >                         goto free_skb;
> > >
> > >                 tnl_params = (const struct iphdr *)skb->data;
> > >
> > > +               if (pull_len > skb_transport_offset(skb))
> > > +                       goto free_skb;
> > > +
> > >                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> > >                  * to gre header.
> > >                  */
> > > -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > > +               skb_pull(skb, pull_len);
> > >                 skb_reset_mac_header(skb);
> > >
> > > plus then
> > >
> > >  static int gre_handle_offloads(struct sk_buff *skb, bool csum)
> > >  {
> > > -       /* Local checksum offload requires csum offload of the inner packet */
> > > -       if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
> > > -           skb_checksum_start(skb) < skb->data)
> > > -               return -EINVAL;
> > > -
> > >         return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM :
> > > SKB_GSO_GRE);
> > >  }
> >
> > Yes, this is what I was thinking. We will also need an IPv6 version of
> > this as well, and may want to add a comment clarifying that this is to
> > prevent us from pointing inner offsets at pulled headers.
> >
> > It lets us drop the csum, ipsummed, and csum_start checks in favor of
> > just the skb_transport_offset comparison which should be a net win
> > since it reduces the number of paths the code is encountered in, and
> > reduces the number of checks to just 1.
>
> Okay. Yes, this looks better to me too. Thanks.
>
> Do you want to submit it? Or I can do it, either way.

You can do it since you have essentially already written half the code.. :)

Thanks.

- Alex

Powered by blists - more mailing lists