Message ID | 20190215171547.247018-1-willemdebruijn.kernel@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: validate untrusted gso packets without csum offload | expand |
On 02/15/2019 09:15 AM, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Syzkaller again found a path to a kernel crash through bad gso input. > By building an excessively large packet to cause an skb field to wrap. > > If VIRTIO_NET_HDR_F_NEEDS_CSUM was set this would have been dropped in > skb_partial_csum_set. > > GSO packets that do not set checksum offload are suspicious and rare. > Most callers of virtio_net_hdr_to_skb already pass them to > skb_probe_transport_header. > > Move that test forward, change it to detect parse failure and drop > packets on failure as those cleary are not one of the legitimate > VIRTIO_NET_HDR_GSO types. > > Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.") > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> Reviewed-by: Eric Dumazet <edumazet@google.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: Fri, 15 Feb 2019 12:15:47 -0500 > From: Willem de Bruijn <willemb@google.com> > > Syzkaller again found a path to a kernel crash through bad gso input. > By building an excessively large packet to cause an skb field to wrap. > > If VIRTIO_NET_HDR_F_NEEDS_CSUM was set this would have been dropped in > skb_partial_csum_set. > > GSO packets that do not set checksum offload are suspicious and rare. > Most callers of virtio_net_hdr_to_skb already pass them to > skb_probe_transport_header. > > Move that test forward, change it to detect parse failure and drop > packets on failure as those cleary are not one of the legitimate > VIRTIO_NET_HDR_GSO types. > > Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.") > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> Applied and queued up for -stable, thanks Willem. > This captures a variety of bad gso packets, but to tighten further: > > - drop SKB_GSO_DODGY packets with ipip/sit/.. , which cannot be legal. > by ipip_gso_segment wrappers around inet_gso_segment > expands on 121d57af308d ("gso: validate gso_type in GSO handlers") > > - limit the number of ipv6 exthdrs allowed from dodgy sources. > not sure where to draw the line. but not at 64K ;) > > - validate the network and transport protocol returned in > skb_probe_transport_header against the VIRTIO_NET_HDR_GSO type > > - probe all dodgy GSO packets, also those that set checksum offload. > this will have a performance impact, discussed previously in > http://patchwork.ozlabs.org/patch/861874/ > but it would have blocked this latest bug as well > > All but the last one seem pretty uncontroversial to me. If no one > objects I plan to send those to net-next. No objections from me.
On Fri, Feb 15, 2019 at 12:15 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > From: Willem de Bruijn <willemb@google.com> > > Syzkaller again found a path to a kernel crash through bad gso input. > By building an excessively large packet to cause an skb field to wrap. > > If VIRTIO_NET_HDR_F_NEEDS_CSUM was set this would have been dropped in > skb_partial_csum_set. > > GSO packets that do not set checksum offload are suspicious and rare. > Most callers of virtio_net_hdr_to_skb already pass them to > skb_probe_transport_header. > > Move that test forward, change it to detect parse failure and drop > packets on failure as those cleary are not one of the legitimate > VIRTIO_NET_HDR_GSO types. > > Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.") > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > Reported-by: syzbot <syzkaller@googlegroups.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> This causes false positive drops on virtio-net and tun for these packets with gso without csum_off. And on pf_packet with proto 0. This happens because skb->protocol is set in these callers after the call to virtio_net_hdr_to_skb. And the flow dissector relies on this to start dissection, not the link layer header (if present). Moving this logic forward is too much churn for net, especially since eth_type_header pulls the header, requiring additional changes to adjust csum_start. virtio_net_hdr_set_proto() aims to fix this by deriving skb->protocol from the gso_type. But unfortunately for UDP it unconditionally selects ipv4, which will cause drops for UDP over ipv6. For net I plan to just ignore the error for these callers that do not set skb->protocol. - if (!skb_transport_header_was_set(skb)) + if (!skb_transport_header_was_set(skb) && skb->protocol) Possibly with an extension of tpacket_set_protocol to also cover packet_snd, so that that cannot evade it on purpose. Other callers can wait till net-next. > > --- > > This captures a variety of bad gso packets, but to tighten further: > > - drop SKB_GSO_DODGY packets with ipip/sit/.. , which cannot be legal. > by ipip_gso_segment wrappers around inet_gso_segment > expands on 121d57af308d ("gso: validate gso_type in GSO handlers") > > - limit the number of ipv6 exthdrs allowed from dodgy sources. > not sure where to draw the line. but not at 64K ;) This already exists, in the form of skb_flow_dissect_allowed > - validate the network and transport protocol returned in > skb_probe_transport_header against the VIRTIO_NET_HDR_GSO type > > - probe all dodgy GSO packets, also those that set checksum offload. > this will have a performance impact, discussed previously in > http://patchwork.ozlabs.org/patch/861874/ > but it would have blocked this latest bug as well > > All but the last one seem pretty uncontroversial to me. If no one > objects I plan to send those to net-next. > > --- > include/linux/skbuff.h | 2 +- > include/linux/virtio_net.h | 9 +++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 95d25b010a25..4c1c82a5678c 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -2434,7 +2434,7 @@ static inline void skb_probe_transport_header(struct sk_buff *skb, > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0)) > skb_set_transport_header(skb, keys.control.thoff); > - else > + else if (offset_hint >= 0) > skb_set_transport_header(skb, offset_hint); > } > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index cb462f9ab7dd..71f2394abbf7 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -57,6 +57,15 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, > > if (!skb_partial_csum_set(skb, start, off)) > return -EINVAL; > + } else { > + /* gso packets without NEEDS_CSUM do not set transport_offset. > + * probe and drop if does not match one of the above types. > + */ > + if (gso_type) { > + skb_probe_transport_header(skb, -1); > + if (!skb_transport_header_was_set(skb)) > + return -EINVAL; > + } > } > > if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) { > -- > 2.21.0.rc0.258.g878e2cd30e-goog >
On Mon, Feb 18, 2019 at 2:12 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 12:15 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > From: Willem de Bruijn <willemb@google.com> > > > > Syzkaller again found a path to a kernel crash through bad gso input. > > By building an excessively large packet to cause an skb field to wrap. > > > > If VIRTIO_NET_HDR_F_NEEDS_CSUM was set this would have been dropped in > > skb_partial_csum_set. > > > > GSO packets that do not set checksum offload are suspicious and rare. > > Most callers of virtio_net_hdr_to_skb already pass them to > > skb_probe_transport_header. > > > > Move that test forward, change it to detect parse failure and drop > > packets on failure as those cleary are not one of the legitimate > > VIRTIO_NET_HDR_GSO types. > > > > Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.") > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > > Reported-by: syzbot <syzkaller@googlegroups.com> > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > This causes false positive drops on virtio-net and tun for these > packets with gso without csum_off. And on pf_packet with proto 0. > > This happens because skb->protocol is set in these callers after > the call to virtio_net_hdr_to_skb. And the flow dissector relies on > this to start dissection, not the link layer header (if present). > Moving this logic forward is too much churn for net, especially since > eth_type_header pulls the header, requiring additional changes to > adjust csum_start. > > virtio_net_hdr_set_proto() aims to fix this by deriving skb->protocol > from the gso_type. But unfortunately for UDP it unconditionally > selects ipv4, which will cause drops for UDP over ipv6. Suggested fix at http://patchwork.ozlabs.org/patch/1044429/
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 95d25b010a25..4c1c82a5678c 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2434,7 +2434,7 @@ static inline void skb_probe_transport_header(struct sk_buff *skb, if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0)) skb_set_transport_header(skb, keys.control.thoff); - else + else if (offset_hint >= 0) skb_set_transport_header(skb, offset_hint); } diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index cb462f9ab7dd..71f2394abbf7 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -57,6 +57,15 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, if (!skb_partial_csum_set(skb, start, off)) return -EINVAL; + } else { + /* gso packets without NEEDS_CSUM do not set transport_offset. + * probe and drop if does not match one of the above types. + */ + if (gso_type) { + skb_probe_transport_header(skb, -1); + if (!skb_transport_header_was_set(skb)) + return -EINVAL; + } } if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {