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.
Toshiaki Makita
