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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 16 Jul 2019 20:33:33 -0700
From:   Petar Penkov <ppenkov.kernel@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        davem@...emloft.net, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Petar Penkov <ppenkov@...gle.com>
Subject: Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

Thank you for your feedback!

On Tue, Jul 16, 2019 at 7:26 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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.

I'll make sure to include this check in the next version.

>
> 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 ?

I did it to mirror bpf_tcp_check_syncookie which sees a network order
cookie. That said, it is probably better for the caller to call
bpf_htonl() as they see fit, rather than to do it in the helper. Will
update, thanks.
>
> 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.
>

Agreed, having an additional argument would have been helpful for this function.