netdev - Re: [PATCH bpf-next v4 6/6] bpf, selftests: add redirect

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:   Wed, 30 Sep 2020 23:01:45 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     ast@...nel.org, john.fastabend@...il.com, kafai@...com,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v4 6/6] bpf, selftests: add redirect_neigh
 selftest

On 9/30/20 9:20 PM, Alexei Starovoitov wrote:
> On Wed, Sep 30, 2020 at 05:18:20PM +0200, Daniel Borkmann wrote:
>> +
>> +#ifndef barrier_data
>> +# define barrier_data(ptr)	asm volatile("": :"r"(ptr) :"memory")
>> +#endif
>> +
>> +#ifndef ctx_ptr
>> +# define ctx_ptr(field)		(void *)(long)(field)
>> +#endif
> 
>> +static __always_inline bool is_remote_ep_v4(struct __sk_buff *skb,
>> +					    __be32 addr)
>> +{
>> +	void *data_end = ctx_ptr(skb->data_end);
>> +	void *data = ctx_ptr(skb->data);
> 
> please consider adding:
>          __bpf_md_ptr(void *, data);
>          __bpf_md_ptr(void *, data_end);
> to struct __sk_buff in a followup to avoid this casting headache.

You mean also for the other ctx types? I can take a look, yeah.

>> +SEC("dst_ingress") int tc_dst(struct __sk_buff *skb)
>> +{
>> +	int idx = dst_to_src_tmp;
>> +	__u8 zero[ETH_ALEN * 2];
>> +	bool redirect = false;
>> +
>> +	switch (skb->protocol) {
>> +	case __bpf_constant_htons(ETH_P_IP):
>> +		redirect = is_remote_ep_v4(skb, __bpf_constant_htonl(ip4_src));
>> +		break;
>> +	case __bpf_constant_htons(ETH_P_IPV6):
>> +		redirect = is_remote_ep_v6(skb, (struct in6_addr)ip6_src);
>> +		break;
>> +	}
>> +
>> +	if (!redirect)
>> +		return TC_ACT_OK;
>> +
>> +	barrier_data(&idx);
>> +	idx = bpf_ntohl(idx);
> 
> I don't follow. Why force that constant into a register and force
> actual swap instruction?
> 
>> +
>> +	__builtin_memset(&zero, 0, sizeof(zero));
>> +	if (bpf_skb_store_bytes(skb, 0, &zero, sizeof(zero), 0) < 0)
>> +		return TC_ACT_SHOT;
>> +
>> +	return bpf_redirect_neigh(idx, 0);
>> +}
> 
>> +xxd -p < test_tc_neigh.o   | sed "s/eeddddee/$veth_src/g" | xxd -r -p > test_tc_neigh.x.o
>> +xxd -p < test_tc_neigh.x.o | sed "s/eeffffee/$veth_dst/g" | xxd -r -p > test_tc_neigh.y.o
> 
> So the inline asm is because of the above?
> So after compiling you're hacking elf binary for this pattern ?
> Ouch. Please use global data or something. This is fragile.
> This type of hacks should be discouraged and having selftests do them
> goes as counter example.

Yeah, so the barrier_data() was to avoid compiler to optimize, and the bpf_ntohl()
to load target ifindex which was stored in big endian. Thanks for applying the set,
I'll look into reworking this to have a loader application w/ the global data and
then to pin it and have iproute2 pick this up from the pinned location, for example
(or directly interact with netlink wrt attaching ... I'll see which is better).

Thanks,
Daniel