[Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toke Høiland-Jørgensen]

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@gmail.com>
Reported-by: Daniel Borkmann <daniel@iogearbox.net>
Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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 */
+
 /*
  * 	struct vlan_hdr - vlan header
  * 	@h_vlan_TCI: priority and VLAN ID
@@ -308,34 +310,6 @@ static inline bool eth_type_vlan(__be16 ethertype)
 	}
 }
 
-/* A getter for the SKB protocol field which will handle VLAN tags consistently
- * whether VLAN acceleration is enabled or not.
- */
-static inline __be16 skb_protocol(const struct sk_buff *skb, bool skip_vlan)
-{
-	unsigned int offset = skb_mac_offset(skb) + sizeof(struct ethhdr);
-	__be16 proto = skb->protocol;
-
-	if (!skip_vlan)
-		/* VLAN acceleration strips the VLAN header from the skb and
-		 * moves it to skb->vlan_proto
-		 */
-		return skb_vlan_tag_present(skb) ? skb->vlan_proto : proto;
-
-	while (eth_type_vlan(proto)) {
-		struct vlan_hdr vhdr, *vh;
-
-		vh = skb_header_pointer(skb, offset, sizeof(vhdr), &vhdr);
-		if (!vh)
-			break;
-
-		proto = vh->h_vlan_encapsulated_proto;
-		offset += sizeof(vhdr);
-	}
-
-	return proto;
-}
-
 static inline bool vlan_hw_offload_capable(netdev_features_t features,
 					   __be16 proto)
 {
@@ -605,10 +579,10 @@ static inline int vlan_get_tag(const struct sk_buff *skb, u16 *vlan_tci)
  * Returns the EtherType of the packet, regardless of whether it is
  * vlan encapsulated (normal or hardware accelerated) or not.
  */
-static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
+static inline __be16 __vlan_get_protocol(const struct sk_buff *skb, __be16 type,
 					 int *depth)
 {
-	unsigned int vlan_depth = skb->mac_len;
+	unsigned int vlan_depth = skb->mac_len, parse_depth = VLAN_MAX_DEPTH;
 
 	/* if type is 802.1Q/AD then the header should already be
 	 * present at mac_len - VLAN_HLEN (if mac_len > 0), or at
@@ -623,13 +597,12 @@ static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
 			vlan_depth = ETH_HLEN;
 		}
 		do {
-			struct vlan_hdr *vh;
+			struct vlan_hdr vhdr, *vh;
 
-			if (unlikely(!pskb_may_pull(skb,
-						    vlan_depth + VLAN_HLEN)))
+			vh = skb_header_pointer(skb, vlan_depth, sizeof(vhdr), &vhdr);
+			if (unlikely(!vh || !--parse_depth))
 				return 0;
 
-			vh = (struct vlan_hdr *)(skb->data + vlan_depth);
 			type = vh->h_vlan_encapsulated_proto;
 			vlan_depth += VLAN_HLEN;
 		} while (eth_type_vlan(type));
@@ -648,11 +621,25 @@ static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
  * Returns the EtherType of the packet, regardless of whether it is
  * vlan encapsulated (normal or hardware accelerated) or not.
  */
-static inline __be16 vlan_get_protocol(struct sk_buff *skb)
+static inline __be16 vlan_get_protocol(const struct sk_buff *skb)
 {
 	return __vlan_get_protocol(skb, skb->protocol, NULL);
 }
 
+/* A getter for the SKB protocol field which will handle VLAN tags consistently
+ * whether VLAN acceleration is enabled or not.
+ */
+static inline __be16 skb_protocol(const struct sk_buff *skb, bool skip_vlan)
+{
+	if (!skip_vlan)
+		/* VLAN acceleration strips the VLAN header from the skb and
+		 * moves it to skb->vlan_proto
+		 */
+		return skb_vlan_tag_present(skb) ? skb->vlan_proto : skb->protocol;
+
+	return vlan_get_protocol(skb);
+}
+
 static inline void vlan_set_encap_proto(struct sk_buff *skb,
 					struct vlan_hdr *vhdr)
 {
-- 
2.27.0

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Daniel Borkmann]

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@gmail.com>
> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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? Why not 8 as max, for
example (I'd probably even consider a depth like this as utterly broken setup ..)?

Thanks,
Daniel

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.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@gmail.com>
>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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/ :)

-Toke

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toshiaki Makita]

On 2020/07/06 21:29, 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@gmail.com>
> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
...
> @@ -623,13 +597,12 @@ static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
>   			vlan_depth = ETH_HLEN;
>   		}
>   		do {
> -			struct vlan_hdr *vh;
> +			struct vlan_hdr vhdr, *vh;
>   
> -			if (unlikely(!pskb_may_pull(skb,
> -						    vlan_depth + VLAN_HLEN)))
> +			vh = skb_header_pointer(skb, vlan_depth, sizeof(vhdr), &vhdr);

Some drivers which use vlan_get_protocol to get IP protocol for checksum offload discards
packets when it cannot get the protocol.
I guess for such users this function should try to get protocol even if it is not in skb header?
I'm not sure such a case can happen, but since you care about this, you know real cases where
vlan tag can be in skb frags?

Toshiaki Makita

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toshiaki Makita]

On 2020/07/07 7:44, Toke Høiland-Jørgensen wrote:
> Daniel Borkmann <daniel@iogearbox.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@gmail.com>
>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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.

Toshiaki Makita

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toke Høiland-Jørgensen]

Toshiaki Makita <toshiaki.makita1@gmail.com> writes:

> On 2020/07/07 7:44, Toke Høiland-Jørgensen wrote:
>> Daniel Borkmann <daniel@iogearbox.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@gmail.com>
>>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toke Høiland-Jørgensen]

Toshiaki Makita <toshiaki.makita1@gmail.com> writes:

> On 2020/07/06 21:29, 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@gmail.com>
>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
> ...
>> @@ -623,13 +597,12 @@ static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
>>   			vlan_depth = ETH_HLEN;
>>   		}
>>   		do {
>> -			struct vlan_hdr *vh;
>> +			struct vlan_hdr vhdr, *vh;
>>   
>> -			if (unlikely(!pskb_may_pull(skb,
>> -						    vlan_depth + VLAN_HLEN)))
>> +			vh = skb_header_pointer(skb, vlan_depth, sizeof(vhdr), &vhdr);
>
> Some drivers which use vlan_get_protocol to get IP protocol for checksum offload discards
> packets when it cannot get the protocol.
> I guess for such users this function should try to get protocol even if it is not in skb header?
> I'm not sure such a case can happen, but since you care about this, you know real cases where
> vlan tag can be in skb frags?

skb_header_pointer() will still succeed in reading the data, it'll just
do so by copying it into the buffer on the stack (vhdr) instead of
moving the SKB data itself around...

-Toke

Re: [Cake] [PATCH net] vlan: consolidate VLAN parsing code and limit max parsing depth

[Toshiaki Makita]

On 2020/07/07 19:57, Toke Høiland-Jørgensen wrote:
> Toshiaki Makita <toshiaki.makita1@gmail.com> writes:
> 
>> On 2020/07/06 21:29, 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@gmail.com>
>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net>
>>> Fixes: d7bf2ebebc2b ("sched: consistently handle layer3 header accesses in the presence of VLANs")
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>> ---
>> ...
>>> @@ -623,13 +597,12 @@ static inline __be16 __vlan_get_protocol(struct sk_buff *skb, __be16 type,
>>>    			vlan_depth = ETH_HLEN;
>>>    		}
>>>    		do {
>>> -			struct vlan_hdr *vh;
>>> +			struct vlan_hdr vhdr, *vh;
>>>    
>>> -			if (unlikely(!pskb_may_pull(skb,
>>> -						    vlan_depth + VLAN_HLEN)))
>>> +			vh = skb_header_pointer(skb, vlan_depth, sizeof(vhdr), &vhdr);
>>
>> Some drivers which use vlan_get_protocol to get IP protocol for checksum offload discards
>> packets when it cannot get the protocol.
>> I guess for such users this function should try to get protocol even if it is not in skb header?
>> I'm not sure such a case can happen, but since you care about this, you know real cases where
>> vlan tag can be in skb frags?
> 
> skb_header_pointer() will still succeed in reading the data, it'll just
> do so by copying it into the buffer on the stack (vhdr) instead of
> moving the SKB data itself around...

True, probably I need some more coffee...
Thanks.

Toshiaki Makita