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

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

[Shreyansh Chouhan]

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? (Which I think
is the most likely solution, however I do not have experience with
networking.) 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

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

[Shreyansh Chouhan]

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;
+
 	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

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

[Willem de Bruijn]

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.

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.

>         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

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

[Shreyansh Chouhan]

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