Re: [PATCH net v3] sched: consistently handle layer3 header accesses in the presence of VLANs

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 7/3/20 10:26 PM, Toke Høiland-Jørgensen wrote:
>> There are a couple of places in net/sched/ that check skb->protocol and act
>> on the value there. However, in the presence of VLAN tags, the value stored
>> in skb->protocol can be inconsistent based on whether VLAN acceleration is
>> enabled. The commit quoted in the Fixes tag below fixed the users of
>> skb->protocol to use a helper that will always see the VLAN ethertype.
>> 
>> However, most of the callers don't actually handle the VLAN ethertype, but
>> expect to find the IP header type in the protocol field. This means that
>> things like changing the ECN field, or parsing diffserv values, stops
>> working if there's a VLAN tag, or if there are multiple nested VLAN
>> tags (QinQ).
>> 
>> To fix this, change the helper to take an argument that indicates whether
>> the caller wants to skip the VLAN tags or not. When skipping VLAN tags, we
>> make sure to skip all of them, so behaviour is consistent even in QinQ
>> mode.
>> 
>> To make the helper usable from the ECN code, move it to if_vlan.h instead
>> of pkt_sched.h.
>> 
>> v3:
>> - Remove empty lines
>> - Move vlan variable definitions inside loop in skb_protocol()
>> - Also use skb_protocol() helper in IP{,6}_ECN_decapsulate() and
>>    bpf_skb_ecn_set_ce()
>> 
>> v2:
>> - Use eth_type_vlan() helper in skb_protocol()
>> - Also fix code that reads skb->protocol directly
>> - Change a couple of 'if/else if' statements to switch constructs to avoid
>>    calling the helper twice
>> 
>> Reported-by: Ilya Ponetayev <i.ponetaev@ndmsystems.com>
>> Fixes: d8b9605d2697 ("net: sched: fix skb->protocol use in case of accelerated vlan path")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>   include/linux/if_vlan.h  | 28 ++++++++++++++++++++++++++++
>>   include/net/inet_ecn.h   | 25 +++++++++++++++++--------
>>   include/net/pkt_sched.h  | 11 -----------
>>   net/core/filter.c        | 10 +++++++---
>>   net/sched/act_connmark.c |  9 ++++++---
>>   net/sched/act_csum.c     |  2 +-
>>   net/sched/act_ct.c       |  9 ++++-----
>>   net/sched/act_ctinfo.c   |  9 ++++++---
>>   net/sched/act_mpls.c     |  2 +-
>>   net/sched/act_skbedit.c  |  2 +-
>>   net/sched/cls_api.c      |  2 +-
>>   net/sched/cls_flow.c     |  8 ++++----
>>   net/sched/cls_flower.c   |  2 +-
>>   net/sched/em_ipset.c     |  2 +-
>>   net/sched/em_ipt.c       |  2 +-
>>   net/sched/em_meta.c      |  2 +-
>>   net/sched/sch_cake.c     |  4 ++--
>>   net/sched/sch_dsmark.c   |  6 +++---
>>   net/sched/sch_teql.c     |  2 +-
>>   19 files changed, 86 insertions(+), 51 deletions(-)
>> 
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index b05e855f1ddd..427a5b8597c2 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -308,6 +308,34 @@ 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);
>> +	}
>
> Hm, why is the while loop 'unbounded'? Does it even make sense to have
> a packet with hundreds of vlan hdrs in there what you'd end up
> walking? What if an attacker crafts a max sized packet with only
> vlan_hdr forcing exorbitant looping in fast-path here (e.g. via
> af_packet)?

Hmm, I guess you're right that could theoretically happen. But on the
other hand, a lot of drivers seem to be cheerfully calling
vlan_get_protocol() on incoming packets, which doesn't have a limit on
the depth either.

I guess I could add a depth limit, but in that case I suppose that
should also be added to vlan_get_protocol() (or the two should be
consolidated). WDYT?

> Did you validate that skb_mac_offset() is always valid for the
> call-sites you converted? (We have a skb_mac_header_was_set() test to
> probe for whether skb->mac_header is set to ~0.)

Not extensively; I kinda assumed it would always be valid at those call
sites, since the callers go on to call ip_hdr() or something similar
right afterwards.

I guess Toshiaki's suggestion to use vlan_get_protocol() could be a way
around this, as that seems to deal with skb->mac_len being 0.

-Toke