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

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

On Thu, Sep 2, 2021 at 4:25 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Thu, Sep 2, 2021 at 12:38 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Only test integrity of csum_start if checksum offload is configured.
> >
> > With checksum offload and GRE tunnel checksum, gre_build_header will
> > cheaply build the GRE checksum using local checksum offload. This
> > depends on inner packet csum offload, and thus that csum_start points
> > behind GRE. But validate this condition only with checksum offload.
> >
> > Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
> > Fixes: 1d011c4803c7 ("ip_gre: add validation for csum_start")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> >  net/ipv4/ip_gre.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > index 177d26d8fb9c..09311992a617 100644
> > --- a/net/ipv4/ip_gre.c
> > +++ b/net/ipv4/ip_gre.c
> > @@ -473,8 +473,11 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
> >
> >  static int gre_handle_offloads(struct sk_buff *skb, bool csum)
> >  {
> > -       if (csum && skb_checksum_start(skb) < skb->data)
> > +       /* 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);
> >  }

Thanks for taking a look.

> So a few minor nits.
>
> First I think we need this for both v4 and v6 since it looks like this
> code is reproduced for net/ipv6/ip6_gre.c.

I sent a separate patch for v6. Perhaps should have made it a series
to make this more clear.

> Second I don't know if we even need to bother including the "csum"
> portion of the lookup since that technically is just telling us if the
> GRE tunnel is requesting a checksum or not and I am not sure that
> applies to the fact that the inner L4 header is going to be what is
> requesting the checksum offload most likely.

This test introduced in the original patch specifically protects the
GRE tunnel checksum calculation using lco_csum. The regular inner
packet path likely is already robust (as similar bug reports would be
more likely for that more common case).

> Also maybe this should be triggering a WARN_ON_ONCE if we hit this as
> the path triggering this should be fixed rather than us silently
> dropping frames. We should be figuring out what cases are resulting in
> us getting CHECKSUM_PARTIAL without skb_checksum_start being set.

We already know that bad packets can enter the kernel and trigger
this, using packet sockets and virtio_net_hdr. Unfortunately, this
*is* the fix.