netdev - Re: [PATCH bpf-next,v2 1/2] bpf: add helper for getting xfrm states

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, 20 Apr 2018 06:43:56 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, shmulik@...anetworks.com, ast@...nel.org,
        daniel@...earbox.net, fw@...len.de, steffen.klassert@...unet.com
Subject: Re: [PATCH bpf-next,v2 1/2] bpf: add helper for getting xfrm states

Hi,

On Wed, 18 Apr 2018 15:31:03 -0700
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> On Thu, Apr 19, 2018 at 12:58:22AM +0300, Eyal Birger wrote:
> > This commit introduces a helper which allows fetching xfrm state
> > parameters by eBPF programs attached to TC.
> > 
> > Prototype:
> > bpf_skb_get_xfrm_state(skb, index, xfrm_state, size, flags)
> > 
> > skb: pointer to skb
> > index: the index in the skb xfrm_state secpath array
> > xfrm_state: pointer to 'struct bpf_xfrm_state'
> > size: size of 'struct bpf_xfrm_state'
> > flags: reserved for future extensions
> > 

<snip>
 
> > +#ifdef CONFIG_XFRM
> > +BPF_CALL_5(bpf_skb_get_xfrm_state, struct sk_buff *, skb, u32,
> > index,
> > +	   struct bpf_xfrm_state *, to, u32, size, u64, flags)
> > +{
> > +	const struct sec_path *sp = skb_sec_path(skb);
> > +	const struct xfrm_state *x;
> > +
> > +	if (!sp || unlikely(index >= sp->len || flags))
> > +		goto err_clear;
> > +
> > +	x = sp->xvec[index];
> > +
> > +	if (unlikely(size != sizeof(struct bpf_xfrm_state)))
> > +		goto err_clear;
> > +
> > +	to->reqid = x->props.reqid;
> > +	to->spi = be32_to_cpu(x->id.spi);
> > +	to->family = x->props.family;
> > +	if (to->family == AF_INET6) {
> > +		memcpy(to->remote_ipv6, x->props.saddr.a6,
> > +		       sizeof(to->remote_ipv6));
> > +	} else {
> > +		to->remote_ipv4 = be32_to_cpu(x->props.saddr.a4);
> > +	}  
> 
> that looks inconsistent. Why v4 is cpu endian, but v6 not?

I agree. I followed the reference in bpf_skb_get_tunnel_key(). 
I can keep v4 in net endianess too.

> Why change endianness of the spi?

I felt it was more consistent with other fields and usually helpful for
programs. I can keep it in network order.

In which case, do you expect it to be typed as __be32 in bpf.h?
(I haven't seen other cases)?

Thanks for your feedback!