Re: Can a valid vnet header have both csum_start and csum_offset 0?

[Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>]

Hi,

Thanks a lot for this explanation.

On Mon, Aug 16, 2021 at 08:17:08AM -0700, Willem de Bruijn wrote:
> On Wed, Aug 11, 2021 at 10:09 PM Shreyansh Chouhan
> <chouhan.shreyansh630@gmail.com> wrote:
> >
> > On Wed, Aug 11, 2021 at 12:36:38AM +0530, Shreyansh Chouhan wrote:
> > > Hi,
> > >
> > > When parsing the vnet header in __packet_snd_vnet_parse[1], we do not
> > > check for if the values of csum_start and csum_offset given in the
> > > header are both 0.
> > >
> > > Having both these values 0, however, causes a crash[2] further down the
> > > gre xmit code path. In the function ipgre_xmit, we pull the ip header
> > > and gre header from skb->data, this results in an invalid
> > > skb->csum_start which was calculated from the vnet header. The
> > > skb->csum_start offset in this case turns out to be lower than
> > > skb->transport_header. This causes us to pass a negative number as an
> > > argument to csum_partial[3] and eventually to do_csum[4], which then causes
> > > a kernel oops in the while loop.
> > >
> > > I do not understand what should the correct behavior be in this
> > > scenario, should we consider this vnet header as invalid?
> >
> > Something like the following diff:
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 57a1971f29e5..65bff1c8f75c 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2479,13 +2479,17 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> >
> >  static int __packet_snd_vnet_parse(struct virtio_net_hdr *vnet_hdr, size_t len)
> >  {
> > +       __u16 csum_start = __virtio16_to_cpu(vio_le(), vnet_hdr->csum_start);
> > +       __u16 csum_offset = __virtio16_to_cpu(vio_le(), vnet_hdr->csum_offset);
> > +       __u16 hdr_len = __virtio16_to_cpu(vio_le(), vnet_hdr->hdr_len);
> > +
> > +       if (csum_start + csum_offset == 0)
> > +               return -EINVAL;
> > +
> 
> Yet another virtio_net_hdr validation issue.
> 
> The issue is not unique to value 0, but true for any csum_start <
> skb->transport_header at the time of the call to lco_csum. That
> function not unreasonably assumes that csum_start >= l4_hdr.
> 

I thought of that, but then when I went further back in the code
execution path, and saw the code that parses the virtio net header, I thought
that if VIRTIO_NET_HDR_F_NEEDS_CSUM then the csum_start and csum_offset
together must point to some valid location inside the packet.

Thanks a lot for explaining this. Looks like I backtracked more than
necessary.

> The packet socket code is protocol agnostic. It does not know that
> this is an IP GRE packet with greh->flags & TUNNEL_CSUM. We cannot add
> checks there.
> 
> This does appear to be specific to the GRE checksum field, so it won't
> be relevant to other ip tunnel devices:
> 
>                 if (flags & TUNNEL_CSUM &&
>                     !(skb_shinfo(skb)->gso_type &
>                       (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) {
>                         *ptr = 0;
>                         if (skb->ip_summed == CHECKSUM_PARTIAL) {
>                                 *(__sum16 *)ptr = csum_fold(lco_csum(skb));
> 
> Then the check can be performed between where the ip header is pulled,
> in ipgre_xmit:
> 
>         if (dev->header_ops) {
>                 if (skb_cow_head(skb, 0))
>                         goto free_skb;
> 
>                 tnl_params = (const struct iphdr *)skb->data;
> 
>                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>                  * to gre header.
>                  */
>                 skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));   <--------
>                 skb_reset_mac_header(skb);
>         } else {
>                 if (skb_cow_head(skb, dev->needed_headroom))
>                         goto free_skb;
> 
>                 tnl_params = &tunnel->parms.iph;
>         }
> 
>         if (gre_handle_offloads(skb, !!(tunnel->parms.o_flags & TUNNEL_CSUM)))
>                 goto free_skb;
> 
>         __gre_xmit(skb, dev, tnl_params, skb->protocol);
> 
> and when gre_build_header resets the transport header and calls
> lco_csum. gre_build_header is called from __gre_xmit in the above.
> 
> gre_handle_offloads might then be a good location. Perhaps:
> 
>     static int gre_handle_offloads(struct sk_buff *skb, bool csum)
>     {
>     +       if (csum && skb->csum_start < skb->data)
>     +               return -EINVAL;
>     +
>             return iptunnel_handle_offloads(skb, csum ?
> SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
>      }
> 
> And same for ip6gre.
> 
> Having to add branches inside the hotpath that normally sees well
> behaved packets coming from the kernel stack, only to be robust
> against with malformed packets with bad virtio_net_hdr, is
> unfortunate. This way, the check is at least limited to GRE packets
> with TUNNEL_CSUM.
> 

I see, I had no idea about this being a hotpath as well. Thank you for
clearing all of this out. This does help in isolating the branches to
gre packets that require a checksum.

I will make a patch according to the diff that you suggested, and send
it.

Regards,
Shreyansh Chouhan

> >         if ((vnet_hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> > -           (__virtio16_to_cpu(vio_le(), vnet_hdr->csum_start) +
> > -            __virtio16_to_cpu(vio_le(), vnet_hdr->csum_offset) + 2 >
> > -             __virtio16_to_cpu(vio_le(), vnet_hdr->hdr_len)))
> > +           csum_start + csum_offset + 2 > hdr_len)
> >                 vnet_hdr->hdr_len = __cpu_to_virtio16(vio_le(),
> > -                        __virtio16_to_cpu(vio_le(), vnet_hdr->csum_start) +
> > -                       __virtio16_to_cpu(vio_le(), vnet_hdr->csum_offset) + 2);
> > +                               csum_start + csum_offset + 2);
> >
> >         if (__virtio16_to_cpu(vio_le(), vnet_hdr->hdr_len) > len)
> >                 return -EINVAL;
> >
> > > Or should we rather accomodate for both csum_start
> > > and csum_offset values to be 0 in ipgre_xmit?
> > >
> > > Regards,
> > > Shreyansh Chouhan
> > >
> > > --
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/packet/af_packet.c#n2480
> > > [2] https://syzkaller.appspot.com/bug?id=c391f74aac26dd8311c45743ae618f9d5e38b674
> > > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/skbuff.h#n4662
> > > [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/lib/csum-partial_64.c#n35