Toshiaki Makita <[email protected]> writes: > On 2020/07/07 7:44, Toke Høiland-Jørgensen wrote: >> Daniel Borkmann <[email protected]> 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 <[email protected]> >>>> Reported-by: Daniel Borkmann <[email protected]> >>>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in >>>> the presence of VLANs") >>>> Signed-off-by: Toke Høiland-Jørgensen <[email protected]> >>>> --- >>>> 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 _______________________________________________ Cake mailing list [email protected] https://lists.bufferbloat.net/listinfo/cake
