Date:   Sat, 4 Sep 2021 17:40:07 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Duyck <alexander.duyck@...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 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);
 }

Powered by blists - more mailing lists