On Tue, Jul 23, 2019 at 10:37:29AM +0100, Lorenz Bauer wrote:
> On Tue, 23 Jul 2019 at 01:20, Petar Penkov <ppenkov.kernel@xxxxxxxxx> wrote:
> > +static __always_inline __s64 gen_syncookie(void *data_end, struct bpf_sock *sk,
> > + void *iph, __u32 ip_size,
> > + struct tcphdr *tcph)
> > +{
> > + __u32 thlen = tcph->doff * 4;
> > +
> > + if (tcph->syn && !tcph->ack) {
> > + // packet should only have an MSS option
> > + if (thlen != 24)
> > + return 0;
>
> Just for my own understanding: without this the verifier complains since
> thlen is not a known value, even though it is in bounds due to the check below?
the verifier understands only constant part of the packet pointer.
Without additional 'if' above the statement:
if ((void *)tcph + thlen > data_end)
will add variables length 'thlen' to pkt pointer which will become
another pkt pointer (with different id).
That pointer would need 'pkt + const_range > data_end' to have valid access.
We hit this issue in the past when folks wanted to use bpf_csum_diff() helper
with variable size.
It's possible to extend the verifier to support that but it's intrusive,
since variable part would need to passed around to a bunch of check* functions.
I think it's tricky, but doable. Looking forward to patches :)
> > +
> > + if ((void *)tcph + thlen > data_end)
> > + return 0;
> > +
> > + return bpf_tcp_gen_syncookie(sk, iph, ip_size, tcph, thlen);
> > + }
> > + return 0;
> > +}
> > +
>
> --
> Lorenz Bauer | Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com
