Re: [PATCH net] ip_gre: validate csum_start only if CHECKSUM_PARTIAL

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Fri, Sep 3, 2021 at 7:27 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Fri, Sep 3, 2021 at 12:38 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.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.

> > > 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.