netdev - Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen

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, 16 Jul 2019 19:26:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Petar Penkov <ppenkov.kernel@...il.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, davem@...emloft.net, ast@...nel.org,
        daniel@...earbox.net, edumazet@...gle.com, lmb@...udflare.com,
        sdf@...gle.com, Petar Penkov <ppenkov@...gle.com>
Subject: Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

On Tue, Jul 16, 2019 at 09:59:26AM +0200, Eric Dumazet wrote:
> 
> 
> On 7/16/19 2:26 AM, Petar Penkov wrote:
> > From: Petar Penkov <ppenkov@...gle.com>
> > 
> > This helper function allows BPF programs to try to generate SYN
> > cookies, given a reference to a listener socket. The function works
> > from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
> > socket in both cases.
> > 
> ...
> >  
> > +BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > +	   struct tcphdr *, th, u32, th_len)
> > +{
> > +#ifdef CONFIG_SYN_COOKIES
> > +	u32 cookie;
> > +	u16 mss;
> > +
> > +	if (unlikely(th_len < sizeof(*th)))
> 
> 
> You probably need to check that th_len == th->doff * 4

+1
that is surely necessary for safety.

Considering the limitation of 5 args the api choice is good.
struct bpf_syncookie approach doesn't look natural to me.
And I couldn't come up with any better way to represent this helper.
So let's go with 
return htonl(cookie) | ((u64)mss << 32);
My only question is why htonl ?

Independently of that...
Since we've been hitting this 5 args limit too much,
we need to start thinking how to extend BPF ISA to pass
args 6,7,8 on stack.