Date:   Sat, 4 Sep 2021 18:05:08 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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 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.

Powered by blists - more mailing lists