netdev - Re: [PATCH bpf-next 3/3] bpf: Add mtu checking to FIB forwarding helper

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 May 2018 16:01:27 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        borkmann@...earbox.net, ast@...nel.org
Cc:     davem@...emloft.net
Subject: Re: [PATCH bpf-next 3/3] bpf: Add mtu checking to FIB forwarding
 helper

On 05/18/2018 02:34 AM, David Ahern wrote:
> On 5/17/18 4:22 PM, Daniel Borkmann wrote:
>> On 05/17/2018 06:09 PM, David Ahern wrote:
>>> Add check that egress MTU can handle packet to be forwarded. If
>>> the MTU is less than the packet lenght, return 0 meaning the
>>> packet is expected to continue up the stack for help - eg.,
>>> fragmenting the packet or sending an ICMP.
>>>
>>> Signed-off-by: David Ahern <dsahern@...il.com>
>>> ---
>>>  net/core/filter.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 6d0d1560bd70..c47c47a75d4b 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -4098,6 +4098,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>  	struct fib_nh *nh;
>>>  	struct flowi4 fl4;
>>>  	int err;
>>> +	u32 mtu;
>>>  
>>>  	dev = dev_get_by_index_rcu(net, params->ifindex);
>>>  	if (unlikely(!dev))
>>> @@ -4149,6 +4150,10 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>  	if (res.fi->fib_nhs > 1)
>>>  		fib_select_path(net, &res, &fl4, NULL);
>>>  
>>> +	mtu = ip_mtu_from_fib_result(&res, params->ipv4_dst);
>>> +	if (params->tot_len > mtu)
>>> +		return 0;
>>> +
>>>  	nh = &res.fi->fib_nh[res.nh_sel];
>>>  
>>>  	/* do not handle lwt encaps right now */
>>> @@ -4188,6 +4193,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>  	struct flowi6 fl6;
>>>  	int strict = 0;
>>>  	int oif;
>>> +	u32 mtu;
>>>  
>>>  	/* link local addresses are never forwarded */
>>>  	if (rt6_need_strict(dst) || rt6_need_strict(src))
>>> @@ -4250,6 +4256,10 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>  						       fl6.flowi6_oif, NULL,
>>>  						       strict);
>>>  
>>> +	mtu = ip6_mtu_from_fib6(f6i, dst, src);
>>> +	if (params->tot_len > mtu)
>>> +		return 0;
>>> +
>>>  	if (f6i->fib6_nh.nh_lwtstate)
>>>  		return 0;
>>
>> Could you elaborate how this interacts in tc BPF use case where you have e.g.
>> GSO packets and tot_len from aggregated packets would definitely be larger
>> than MTU (e.g. see is_skb_forwardable() as one example on such checks)? Should
>> this be an opt-in via a new flag for the helper?
> 
> It should not be opt-in for XDP.

Yes, correct, for XDP it should not.

> I could add a flag to the internal call -- bpf_skb_fib_lookup sets the
> flag to skip the MTU check in bpf_ipv4_fib_lookup and bpf_ipv6_fib_lookup.
> 
> For the skb case do you want bpf_skb_fib_lookup call is_skb_forwardable
> or leave that to the BPF program?

I think it probably makes sense to add an internal (unexposed) flag or bool
where we propagate skb_is_gso(skb) from bpf_skb_fib_lookup() call-site and
have similar logic where we first check this bool and if false do the MTU
check (so it still can get enforced for control packets). Thus probably nothing
of that implementation detail needs to be exposed to the program author.