netdev - Re: [PATCH v15 ] net/veth/XDP: Line-rate packet forwarding in kernel

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:   Tue, 3 Apr 2018 11:14:00 -0600
From:   David Ahern <dsahern@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        "Md. Islam" <mislam4@...t.edu>, netdev@...r.kernel.org,
        David Miller <davem@...emloft.net>, stephen@...workplumber.org,
        agaceph@...il.com, Pavel Emelyanov <xemul@...nvz.org>,
        Eric Dumazet <edumazet@...gle.com>, brouer@...hat.com
Subject: Re: [PATCH v15 ] net/veth/XDP: Line-rate packet forwarding in kernel

On 4/3/18 11:06 AM, Alexei Starovoitov wrote:
>> For 3 and 4 above I was referring to the route lookup part of it; sorry
>> for not being clear.
>>
>> For example, eth1 is enslaved to bond1 which is in VRF red. The lookup
>> needs to go to the table associated with the VRF. That is not known by
>> just looking at eth1. The code exists to walk the upper layers and do
>> the effective translations, just need to cover those cases.
>>
>> The VLAN part of it is a bit more difficult - ingress device for the
>> lookup should be eth1.100 for example not eth1, and then if eth1.100 is
>> enslaved to a VRF, ...
>>
>> None of it is that complex, just need to walk through the various use
>> cases and make sure bpf_ipv4_fwd_lookup and bpf_ipv6_fwd_lookup can do
>> the right thing for these common use cases.
> I'm a bit lost here. Why this is a concern?
> 'index' as argument that bpf prog is passing into the helper.
> The clsbpf program may choose to pass ifindex of the netdev
> it's attached to or some other one.
> In your patch you have:
> +BPF_CALL_3(bpf_ipv4_fwd_lookup, int, index, const struct iphdr *, iph,
> +	   struct ethhdr *, eth)
> +{
> +	struct flowi4 fl4 = {
> +		.daddr = iph->daddr,
> +		.saddr = iph->saddr,
> +		.flowi4_iif = index,
> +		.flowi4_tos = iph->tos & IPTOS_RT_MASK,
> +		.flowi4_scope = RT_SCOPE_UNIVERSE,
> +	};
> 
> As you saying there is concern with .flowi4_iif = index line ?

yes. BPF / XDP programs are installed on the bottom device ... e.g.,
eth1. The L3 lookup is not necessarily done on that device index.


> In the above the only thing Daniel and myself pointed out that
> passing struct iphdr * like this is not safe.
> We either need size argument which would be a bit cumbersome or
> extend verifier a little to specify size as part of helper proto,
> so that verifier can eforce it without having program to pass it.
> imo that's the only bit missing from that patch to upstream it.

sure. I did not mean that item 1. was a big deal, just something that
needed to be fixed.

> 
> Also the helper isn't really related to XDP. It should work as-is
> for clsbpf and xdp programs as far as I can tell.
> 

yes.