From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 297633B2A4 for ; Mon, 6 Jul 2020 18:44:19 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594075458; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WaRcfmh0dnJaXCAB+hSg5BA0paManfn3RR3dWJMFDRw=; b=DT4MffFe5+p2/bdg9fOwZ5FBlGkPyF1jeSUIspVYQUouL4xZHGbbeRvBVr/pC0SljWaVZn kaNHhF3i5wGSU8bAuc1LPQMvaw0iRLW7ZFp9JTFUwUxPJyoM1hEdEtebwjYbfAzbSb/VpA xrXZ0J9GMQFu9uGsk6z3IPh0DMny+ms= Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-414-ERvnBNH2PBWkDtO0wX_F4w-1; Mon, 06 Jul 2020 18:44:15 -0400 X-MC-Unique: ERvnBNH2PBWkDtO0wX_F4w-1 Received: by mail-pg1-f200.google.com with SMTP id u16so30839187pgj.17 for ; Mon, 06 Jul 2020 15:44:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version:content-transfer-encoding; bh=lADiN9digWx1yABnbEC/NhleCjcMB30P02hQYoJI214=; b=BSul0s8hnWufbLBRTuGCvYKTZZqaDUkGlIuADqrwVMbJrI+xGWNjlNrcspiUwCjM4g 553UsmAbluYyb+hJ7M1ga4/Zgh5J5bqzHMyn0ZYunJOCA3avLp8SIUJk7iaDkkaYi1AD U4V8YJteD9iKHMnraHNCPdQ659Yf87W1ReQ4FPxlvKSTNo/9iV5JLCOHKuvW7uNmjj2E Wj3tHQAxzSwXiwOsuWbOg8uxsDm0QVgtLak5Ip0jyX2t5KLC29t2ouZuTccYCcz0t7Yq MbLybJHhnaHYF+W7mDD5JtkApdUtWPW2eLeEtSIhUXC0ejlRpqG2VfvmcBQHqK6JeKSm NF4Q== X-Gm-Message-State: AOAM530gNSNCO4vHDWurp1DohnLJMC2mfJJ9Jri+yrns72DRjP0tmJkT DRqHqazMagP6go7h/lHlsoOpG2zcsqc8P1W1UnB6qY6dwCarJEJb7tOVxjskSc5AEoXGePZMBWz MEkTiV0VYHmZRZwWHSdBleA== X-Received: by 2002:a17:902:bb81:: with SMTP id m1mr42577212pls.134.1594075454291; Mon, 06 Jul 2020 15:44:14 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyKlDUILrNhEZOu4t20bsIU72SPNfPkiSMv6ptgB8+VNWOgOwWU1av6fzMV5gPPxVbpsQ1m2A== X-Received: by 2002:a17:902:bb81:: with SMTP id m1mr42577202pls.134.1594075453960; Mon, 06 Jul 2020 15:44:13 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id v11sm10277248pfc.108.2020.07.06.15.44.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jul 2020 15:44:13 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 58C6B1804ED; Tue, 7 Jul 2020 00:44:08 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Daniel Borkmann , davem@davemloft.net Cc: netdev@vger.kernel.org, cake@lists.bufferbloat.net, Davide Caratti , Jiri Pirko , Jamal Hadi Salim , Cong Wang , Toshiaki Makita In-Reply-To: <4f7b2b71-8b2a-3aea-637d-52b148af1802@iogearbox.net> References: <20200706122951.48142-1-toke@redhat.com> <4f7b2b71-8b2a-3aea-637d-52b148af1802@iogearbox.net> X-Clacks-Overhead: GNU Terry Pratchett Date: Tue, 07 Jul 2020 00:44:08 +0200 Message-ID: <87lfjwl0w7.fsf@toke.dk> MIME-Version: 1.0 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=toke@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jul 2020 22:44:19 -0000 Daniel Borkmann writes: > On 7/6/20 2:29 PM, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Toshiaki pointed out that we now have two very similar functions to extr= act >> the L3 protocol number in the presence of VLAN tags. And Daniel pointed = out >> that the unbounded parsing loop makes it possible for maliciously crafte= d >> packets to loop through potentially hundreds of tags. >>=20 >> Fix both of these issues by consolidating the two parsing functions and >> limiting the VLAN tag parsing to an arbitrarily-chosen, but hopefully >> conservative, max depth of 32 tags. As part of this, switch over >> __vlan_get_protocol() to use skb_header_pointer() instead of >> pskb_may_pull(), to avoid the possible side effects of the latter and ke= ep >> the skb pointer 'const' through all the parsing functions. >>=20 >> Reported-by: Toshiaki Makita >> Reported-by: Daniel Borkmann >> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses = in the presence of VLANs") >> Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen >> --- >> include/linux/if_vlan.h | 57 ++++++++++++++++------------------------- >> 1 file changed, 22 insertions(+), 35 deletions(-) >>=20 >> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h >> index 427a5b8597c2..855d16192e6a 100644 >> --- a/include/linux/if_vlan.h >> +++ b/include/linux/if_vlan.h >> @@ -25,6 +25,8 @@ >> #define VLAN_ETH_DATA_LEN=091500=09/* Max. octets in payload=09 */ >> #define VLAN_ETH_FRAME_LEN=091518=09/* Max. octets in frame sans FCS *= / >> =20 >> +#define VLAN_MAX_DEPTH=0932=09=09/* Max. number of nested VLAN tags par= sed */ >> + > > Any insight on limits of nesting wrt QinQ, maybe from spec side? Don't think so. Wikipedia says this: 802.1ad is upward compatible with 802.1Q. Although 802.1ad is limited to two tags, there is no ceiling on the standard limiting a single frame to more than two tags, allowing for growth in the protocol. In practice Service Provider topologies often anticipate and utilize frames having more than two tags. > Why not 8 as max, for example (I'd probably even consider a depth like > this as utterly broken setup ..)? I originally went with 8, but chickened out after seeing how many places call the parsing function. While I do agree that eight tags is... somewhat excessive... I was trying to make absolutely sure no one would hit this limit in normal use. See also https://xkcd.com/1172/ :) -Toke