From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from www62.your-server.de (www62.your-server.de [213.133.104.62]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 6A7593B2A4 for ; Mon, 6 Jul 2020 16:02:11 -0400 (EDT) Received: from sslproxy05.your-server.de ([78.46.172.2]) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1jsXJP-0002nL-MO; Mon, 06 Jul 2020 22:01:59 +0200 Received: from [178.196.57.75] (helo=pc-9.home) by sslproxy05.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jsXJP-000BEO-F9; Mon, 06 Jul 2020 22:01:59 +0200 To: =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= , davem@davemloft.net Cc: netdev@vger.kernel.org, cake@lists.bufferbloat.net, Davide Caratti , Jiri Pirko , Jamal Hadi Salim , Cong Wang , Toshiaki Makita References: <20200706122951.48142-1-toke@redhat.com> From: Daniel Borkmann Message-ID: <4f7b2b71-8b2a-3aea-637d-52b148af1802@iogearbox.net> Date: Mon, 6 Jul 2020 22:01:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20200706122951.48142-1-toke@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.102.3/25865/Mon Jul 6 16:07:44 2020) 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 20:02:11 -0000 On 7/6/20 2:29 PM, Toke Høiland-Jørgensen wrote: > Toshiaki pointed out that we now have two very similar functions to extract > the L3 protocol number in the presence of VLAN tags. And Daniel pointed out > that the unbounded parsing loop makes it possible for maliciously crafted > packets to loop through potentially hundreds of tags. > > 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 keep > the skb pointer 'const' through all the parsing functions. > > 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øiland-Jørgensen > --- > include/linux/if_vlan.h | 57 ++++++++++++++++------------------------- > 1 file changed, 22 insertions(+), 35 deletions(-) > > 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 1500 /* Max. octets in payload */ > #define VLAN_ETH_FRAME_LEN 1518 /* Max. octets in frame sans FCS */ > > +#define VLAN_MAX_DEPTH 32 /* Max. number of nested VLAN tags parsed */ > + Any insight on limits of nesting wrt QinQ, maybe from spec side? Why not 8 as max, for example (I'd probably even consider a depth like this as utterly broken setup ..)? Thanks, Daniel