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=-3.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS, URIBL_BLOCKED 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 0F153C43381 for ; Fri, 22 Feb 2019 14:17:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CB948206DD for ; Fri, 22 Feb 2019 14:17:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="SpRDtl8l" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726362AbfBVORQ (ORCPT ); Fri, 22 Feb 2019 09:17:16 -0500 Received: from mail-ed1-f68.google.com ([209.85.208.68]:43735 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726131AbfBVORQ (ORCPT ); Fri, 22 Feb 2019 09:17:16 -0500 Received: by mail-ed1-f68.google.com with SMTP id m35so1852635ede.10 for ; Fri, 22 Feb 2019 06:17:15 -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=NOZblrsWyhDUp7utdRJZdVJ3zp5SyvHGriDD7aP9USA=; b=SpRDtl8lnIoJK2EXwNtidNkWAPgMEJsBR/l3O8AjZ/ODo9rTBmU3SpybSer6zS5nwU YKNFYRG4DzAMO8fSswMWzmQ9k+o9MAM9I/zNoLii5/YR7LhAk0t3yBUvgZkj7pICYdVc KOMB+Kk1329mTQlOU9e1QuBXnJ+JlaPOAgRKbpeqGOjJyrx14Ma8iWBWvnXR3eDHxAoD JdybYfPXAHXaNJOwwBC0ann5msuVHPUw52+7ySa5ReMzss+c73sOgZHpPhW95XCL7gkO QBVRc9d8LYsXJbccnL0SA1VbMo8i4BGyWBVj98bOxBBorsDNMfY/hz+jMeWcVsBmRLnr u00A== 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=NOZblrsWyhDUp7utdRJZdVJ3zp5SyvHGriDD7aP9USA=; b=sicvBzlGDoW7M8q3tK9dpTsIC8JSVjGs3MeSwBzZzX1489brBQSm3UZEQa8jaRsau+ cg4fF1XZBhXdJ/7NNmqVump5K6lqPYo4BBnh9GzRTirXr1p97qOivfqPsKS3Gnj0pK6l hEgSYPCgN5aMlsmQwYjxupXfHdg71ut9RXCaOK7JNo3GQIvM3xeHafP5zqL00FQwM4eT XHgZb3uBXfYK+rx4vXJLU9oPL4BH4OlAliO/WfkDvc3AaYKky9y32OQuSzmcK7LusSW/ jiCcn3payIHbM2buhR5vHGI1QH03+bEdCR9ldkLH8wSm30OWrWC3GwmQahc7qFeMZqMo qgbw== X-Gm-Message-State: AHQUAuaW/Mao9L7enMR9C9lAbgq36l/70LcTTTb7u1/VmcWGyjDhqTlM UICtuMdlYre8MDt+5rlyyMctXkO6BtPmfAWsPY4= X-Google-Smtp-Source: AHgI3IZDFoo/j+oEZIu+5PzX/EfQ/y3vqfjSc2fGjmwytfqfquEzUl6Y4SbxbJKM5if64M72G90a3Rdhdx7daccQVno= X-Received: by 2002:aa7:c153:: with SMTP id r19mr3401375edp.139.1550845034580; Fri, 22 Feb 2019 06:17:14 -0800 (PST) MIME-Version: 1.0 References: <20190221123908.7196-1-maximmi@mellanox.com> <20190221123908.7196-2-maximmi@mellanox.com> In-Reply-To: From: Willem de Bruijn Date: Fri, 22 Feb 2019 09:16:37 -0500 Message-ID: Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to invalid value To: Maxim Mikityanskiy Cc: "David S. Miller" , Saeed Mahameed , Willem de Bruijn , Jason Wang , Eric Dumazet , "netdev@vger.kernel.org" , Eran Ben Elisha , Tariq Toukan 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 22, 2019 at 7:30 AM Maxim Mikityanskiy wrote: > > > -----Original Message----- > > From: Willem de Bruijn > > Sent: 21 February, 2019 19:28 > > To: Maxim Mikityanskiy > > Cc: David S. Miller ; Saeed Mahameed > > ; Willem de Bruijn ; Jason Wang > > ; Eric Dumazet ; > > netdev@vger.kernel.org; Eran Ben Elisha ; Tariq Toukan > > > > Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to > > invalid value > > > > On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy > > wrote: > > > > > > If the socket was created with socket(AF_PACKET, SOCK_RAW, 0), > > > skb->protocol will be unset, __skb_flow_dissect() will fail, and > > > skb_probe_transport_header() will fall back to the offset_hint, making > > > the resulting skb_transport_offset incorrect. > > > > > > If, however, there is no transport header in the packet, > > > transport_header shouldn't be set to an arbitrary value. > > > > > > Fix it by leaving the transport offset unset if it couldn't be found, to > > > be explicit rather than to fill it with some wrong value. It changes the > > > behavior, but if some code relied on the old behavior, it would be > > > broken anyway, as the old one is incorrect. > > > > > > Signed-off-by: Maxim Mikityanskiy > > > > qdisc_pkt_len_init also expects skb_transport_header(skb) to always be > > set for gso packets. > > > > Once net is merged into net-next, commit d5be7f632bad ("net: validate > > This commit is already in net-next, isn't it? It is now yes. > > untrusted gso packets without csum offload") will ensure that packets > > that fail flow dissection do not make it into the stack. But we have > > to skip dissection in some cases, like tun [1]. > > OK, got you. However, is everything OK with patch [1]? It fixes false > positives, when a packet was dropped because network_header had not been > set yet for dissection to succeed, but what about evil packets that have > no network_offset at the moment of calling virtio_net_hdr_to_skb? Why > are all of them considered valid? They are not considered valid. But it is the cost of avoiding false positives. We cannot break legitimate traffic. The two patches as is restrict a large class of illegal packets from packet sockets. Which always set network header, so this cannot be worked around. Extending these checks to tun will require an independent additional fix. > > I think we need to add a check in qdisc_pkt_len_init to skip the gso > > size estimation branch if !skb_transport_header_was_set(skb). > > > > Otherwise this patch set looks good to me. To avoid resubmitting > > everything we can fix up the qdisc_pkt_len_init in a follow-up, in > > which case I'm happy to add my Acked-by to this series. > > I'll add this check and submit the patch soon. Thanks for reviewing! > > > [1] http://patchwork.ozlabs.org/patch/1044429/ Looks good, thanks.