netdev - Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib

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:   Mon, 8 Feb 2021 16:41:24 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        colrack@...il.com, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib_lookup helper MTU check
 for SKB ctx

On 2/8/21 4:20 PM, Jesper Dangaard Brouer wrote:
> On Mon, 8 Feb 2021 14:57:13 +0100
> Jesper Dangaard Brouer <brouer@...hat.com> wrote:
>> On Fri, 5 Feb 2021 01:06:35 +0100
>> Daniel Borkmann <daniel@...earbox.net> wrote:
>>> On 2/2/21 5:26 PM, Jesper Dangaard Brouer wrote:
>>>> BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use
>>>> bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size,
>>>> by adjusting fib_params 'tot_len' with the packet length plus the expected
>>>> encap size. (Just like the bpf_check_mtu helper supports). He discovered
>>>> that for SKB ctx the param->tot_len was not used, instead skb->len was used
>>>> (via MTU check in is_skb_forwardable() that checks against netdev MTU).
>>>>
>>>> Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g.
>>>> zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU
>>>> check is done like XDP code-path, which checks against FIB-dst MTU.
[...]
>>>> -	if (!rc) {
>>>> -		struct net_device *dev;
>>>> -
>>>> -		dev = dev_get_by_index_rcu(net, params->ifindex);
>>>> +	if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) {
>>>> +		/* When tot_len isn't provided by user,
>>>> +		 * check skb against net_device MTU
>>>> +		 */
>>>>    		if (!is_skb_forwardable(dev, skb))
>>>>    			rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
>>>
>>> ... so using old cached dev from above will result in wrong MTU check &
>>> subsequent passing of wrong params->mtu_result = dev->mtu this way.
>>
>> Yes, you are right, params->ifindex have a chance to change in the calls.
>> So, our attempt to save an ifindex lookup (dev_get_by_index_rcu) is not
>> correct.
>>
>>> So one
>>> way to fix is that we would need to pass &dev to bpf_ipv{4,6}_fib_lookup().
>>
>> Ok, I will try to code it up, and see how ugly it looks, but I'm no
>> longer sure that it is worth saving this ifindex lookup, as it will
>> only happen if BPF-prog didn't specify params->tot_len.
> 
> I guess we can still do this as an "optimization", but the dev/ifindex
> will very likely be another at this point.

I would say for sake of progress, lets ship your series w/o this optimization so
it can land, and we revisit this later on independent from here. Actually DavidA
back then acked the old poc patch I posted, so maybe that's worth a revisit as
well but needs more testing first.

Thanks,
Daniel