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

[Alexander Duyck <alexander.duyck@gmail.com>]

On Sat, Sep 4, 2021 at 7:46 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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.

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.