lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 07 Jul 2020 12:54:56 +0200 From: Toke Høiland-Jørgensen <toke@...hat.com> To: Toshiaki Makita <toshiaki.makita1@...il.com>, Daniel Borkmann <daniel@...earbox.net>, davem@...emloft.net Cc: netdev@...r.kernel.org, cake@...ts.bufferbloat.net, Davide Caratti <dcaratti@...hat.com>, Jiri Pirko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com> Subject: Re: [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth Toshiaki Makita <toshiaki.makita1@...il.com> writes: > On 2020/07/07 7:44, Toke Høiland-Jørgensen wrote: >> Daniel Borkmann <daniel@...earbox.net> writes: >>> 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 <toshiaki.makita1@...il.com> >>>> Reported-by: Daniel Borkmann <daniel@...earbox.net> >>>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs") >>>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com> >>>> --- >>>> 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? >> >> 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/ :) > > Considering that XMIT_RECURSION_LIMIT is 8, I also think 8 is sufficient. Alright, fair enough, I'll send a v2 with a limit of 8 :) -Toke
Powered by blists - more mailing lists