LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Shreyansh Chouhan <chouhan.shreyansh630@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: Mon, 16 Aug 2021 08:17:08 -0700	[thread overview]
Message-ID: <CAF=yD-JqJAGY9x8LzVhK13R0-nZvwu5uDGKENoEu8AUSrsYX-g@mail.gmail.com> (raw)
In-Reply-To: <YRSlXr2bsFfZOBgw@fedora>

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

  reply	other threads:[~2021-08-16 15:17 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 [this message]
2021-08-18  5:12     ` Shreyansh Chouhan

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='CAF=yD-JqJAGY9x8LzVhK13R0-nZvwu5uDGKENoEu8AUSrsYX-g@mail.gmail.com' \
    --to=willemdebruijn.kernel@gmail.com \
    --cc=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=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).