From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 277B3C43381 for ; Mon, 18 Feb 2019 19:12:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DFD6121773 for ; Mon, 18 Feb 2019 19:12:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="AFr9fzkG" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729004AbfBRTM5 (ORCPT ); Mon, 18 Feb 2019 14:12:57 -0500 Received: from mail-ed1-f67.google.com ([209.85.208.67]:39602 "EHLO mail-ed1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728936AbfBRTMy (ORCPT ); Mon, 18 Feb 2019 14:12:54 -0500 Received: by mail-ed1-f67.google.com with SMTP id p27so7183872edc.6 for ; Mon, 18 Feb 2019 11:12:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Wn0i8YmvlrnqdU2KgFYVDDf5KFCQEpL7/h77Yj8IlSw=; b=AFr9fzkGL3p4EXew7A9AUVpQysCntd1+U90IQFO2efLrR69s400vTFLDIu1jVwY/M5 oTgfY48pY84D6YHBxI9K95WrSo1ZKB555oC99AZ2oUYdOKPQeG8waCniE/hNvZK2sx39 EvX7bbcwAzN9lsADx4MlREMLXsJ0UaO50DIqeb3lJevBN3pOX6pgxDOeZ6ln8hW1pHva 2h3L5QQ2kw5d57UhIc+BAXPUeXtoainNVYEk82G5WOFIXZrTbX2pX/TK4AaF3E/GteAy 5kau9F6qIoFLC9qFKkJgfhxefFoURgICE2r2Bb7uu3lnUwxEy8YUl0bfRlb19VZuYizv XGKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Wn0i8YmvlrnqdU2KgFYVDDf5KFCQEpL7/h77Yj8IlSw=; b=rNJe4Ko8/Tj8bzR2I7tU5P4ZMsg2aeouqQj9gtUUYb9oiz3p2TuAr/d6zZuZneufLn OFvzG7oyfBUWwyf1xW1jl8QHZycBc4sckpi99e2fRdiKhsY74ju3J6Kqgqem0mw7hP0E 9e/gB9LMOMv8b2PYd7yXbH6BxUcqAWiMJU/PzdmWDbJH+0a0+kx9B6ZtqNmZ3qFEQsnj nu51RuTbY0vg9LhX8DIPCmLO8OnfVdAeIRD9EzjMouGeh6SjWvjISYeRWybpL6+FCaP8 IiVEQeHmkBIvYPxI/TNYs/6H+3j0d/KL2zsmAsOLvaL3khEUHiKA45zIcWlqnPDP5vR0 EIEg== X-Gm-Message-State: AHQUAua09cTUxzGWZPfUZzs9JrwRt2hma+wnMnsPw8hzXo+nF4xc28nV wTe5LtgLadYemAz3ApF29TYwaA7TY2TEFcDRPjLMmA== X-Google-Smtp-Source: AHgI3IZ+I/lsiirDFuieXrc80PZw7GTrI8T7KiquZ4uMgyF6oPLh2x0YvDAhnfEhHUSMoFiXtIDI+2GAncfG4J+9LxY= X-Received: by 2002:a17:906:3055:: with SMTP id d21mr17739638ejd.211.1550517171809; Mon, 18 Feb 2019 11:12:51 -0800 (PST) MIME-Version: 1.0 References: <20190215171547.247018-1-willemdebruijn.kernel@gmail.com> In-Reply-To: <20190215171547.247018-1-willemdebruijn.kernel@gmail.com> From: Willem de Bruijn Date: Mon, 18 Feb 2019 14:12:15 -0500 Message-ID: Subject: Re: [PATCH net] net: validate untrusted gso packets without csum offload To: Network Development Cc: David Miller , Eric Dumazet , Jason Wang , Maxim Mikityanskiy , Willem de Bruijn , syzbot Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Feb 15, 2019 at 12:15 PM Willem de Bruijn wrote: > > From: Willem de Bruijn > > 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 > Signed-off-by: Willem de Bruijn 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 >