* Can a valid vnet header have both csum_start and csum_offset 0? @ 2021-08-10 19:06 Shreyansh Chouhan 2021-08-12 4:36 ` Shreyansh Chouhan 0 siblings, 1 reply; 4+ messages in thread From: Shreyansh Chouhan @ 2021-08-10 19:06 UTC (permalink / raw) To: davem, kuba, edumazet, willemb, xie.he.0141, gustavoars, wanghai38, tannerlove, eyal.birger, rsanger, jiapeng.chong Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Can a valid vnet header have both csum_start and csum_offset 0? 2021-08-10 19:06 Can a valid vnet header have both csum_start and csum_offset 0? Shreyansh Chouhan @ 2021-08-12 4:36 ` Shreyansh Chouhan 2021-08-16 15:17 ` Willem de Bruijn 0 siblings, 1 reply; 4+ messages in thread From: Shreyansh Chouhan @ 2021-08-12 4:36 UTC (permalink / raw) To: davem, kuba, edumazet, willemb, xie.he.0141, gustavoars, wanghai38, tannerlove, eyal.birger, rsanger, jiapeng.chong Cc: netdev, linux-kernel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Can a valid vnet header have both csum_start and csum_offset 0? 2021-08-12 4:36 ` Shreyansh Chouhan @ 2021-08-16 15:17 ` Willem de Bruijn 2021-08-18 5:12 ` Shreyansh Chouhan 0 siblings, 1 reply; 4+ messages in thread From: Willem de Bruijn @ 2021-08-16 15:17 UTC (permalink / raw) To: Shreyansh Chouhan Cc: David Miller, Jakub Kicinski, Eric Dumazet, Willem de Bruijn, Xie He, Gustavo A . R . Silva, Wang Hai, Tanner Love, Eyal Birger, Richard Sanger, jiapeng.chong, Network Development, LKML, Jason Wang 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Can a valid vnet header have both csum_start and csum_offset 0? 2021-08-16 15:17 ` Willem de Bruijn @ 2021-08-18 5:12 ` Shreyansh Chouhan 0 siblings, 0 replies; 4+ messages in thread From: Shreyansh Chouhan @ 2021-08-18 5:12 UTC (permalink / raw) To: Willem de Bruijn Cc: David Miller, Jakub Kicinski, Eric Dumazet, Willem de Bruijn, Xie He, Gustavo A . R . Silva, Wang Hai, Tanner Love, Eyal Birger, Richard Sanger, jiapeng.chong, Network Development, LKML, Jason Wang 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-18 5:13 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-10 19:06 Can a valid vnet header have both csum_start and csum_offset 0? 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 is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.