LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Willem de Bruijn <willemb@google.com>,
	Xie He <xie.he.0141@gmail.com>,
	"Gustavo A . R . Silva" <gustavoars@kernel.org>,
	Wang Hai <wanghai38@huawei.com>,
	Tanner Love <tannerlove@google.com>,
	Eyal Birger <eyal.birger@gmail.com>,
	Richard Sanger <rsanger@wand.net.nz>,
	jiapeng.chong@linux.alibaba.com,
	Network Development <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Jason Wang <jasowang@redhat.com>
Subject: Re: Can a valid vnet header have both csum_start and csum_offset 0?
Date: Wed, 18 Aug 2021 10:42:57 +0530	[thread overview]
Message-ID: <YRyW2c/yxcSq8S4O@fedora> (raw)
In-Reply-To: <CAF=yD-JqJAGY9x8LzVhK13R0-nZvwu5uDGKENoEu8AUSrsYX-g@mail.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

      reply	other threads:[~2021-08-18  5:13 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-10 19:06 Shreyansh Chouhan
2021-08-12  4:36 ` Shreyansh Chouhan
2021-08-16 15:17   ` Willem de Bruijn
2021-08-18  5:12     ` Shreyansh Chouhan [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YRyW2c/yxcSq8S4O@fedora \
    --to=chouhan.shreyansh630@gmail.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=eyal.birger@gmail.com \
    --cc=gustavoars@kernel.org \
    --cc=jasowang@redhat.com \
    --cc=jiapeng.chong@linux.alibaba.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=rsanger@wand.net.nz \
    --cc=tannerlove@google.com \
    --cc=wanghai38@huawei.com \
    --cc=willemb@google.com \
    --cc=willemdebruijn.kernel@gmail.com \
    --cc=xie.he.0141@gmail.com \
    --subject='Re: Can a valid vnet header have both csum_start and csum_offset 0?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).