netdev - Re: [bpf-next PATCH 2/3] bpf: fix running sk

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, 02 Jun 2020 10:55:11 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        alexei.starovoitov@...il.com, daniel@...earbox.net
Subject: Re: [bpf-next PATCH 2/3] bpf: fix running sk_skb program types with ktls

On Mon, Jun 01, 2020 at 05:50 PM CEST, John Fastabend wrote:
> John Fastabend wrote:
>> Jakub Sitnicki wrote:
>> > On Fri, 29 May 2020 16:06:59 -0700
>> > John Fastabend <john.fastabend@...il.com> wrote:
>> >
>> > > KTLS uses a stream parser to collect TLS messages and send them to
>> > > the upper layer tls receive handler. This ensures the tls receiver
>> > > has a full TLS header to parse when it is run. However, when a
>> > > socket has BPF_SK_SKB_STREAM_VERDICT program attached before KTLS
>> > > is enabled we end up with two stream parsers running on the same
>> > > socket.
>> > >
>> > > The result is both try to run on the same socket. First the KTLS
>> > > stream parser runs and calls read_sock() which will tcp_read_sock
>> > > which in turn calls tcp_rcv_skb(). This dequeues the skb from the
>> > > sk_receive_queue. When this is done KTLS code then data_ready()
>> > > callback which because we stacked KTLS on top of the bpf stream
>> > > verdict program has been replaced with sk_psock_start_strp(). This
>> > > will in turn kick the stream parser again and eventually do the
>> > > same thing KTLS did above calling into tcp_rcv_skb() and dequeuing
>> > > a skb from the sk_receive_queue.
>> > >
>> > > At this point the data stream is broke. Part of the stream was
>> > > handled by the KTLS side some other bytes may have been handled
>> > > by the BPF side. Generally this results in either missing data
>> > > or more likely a "Bad Message" complaint from the kTLS receive
>> > > handler as the BPF program steals some bytes meant to be in a
>> > > TLS header and/or the TLS header length is no longer correct.
>> > >
>> > > We've already broke the idealized model where we can stack ULPs
>> > > in any order with generic callbacks on the TX side to handle this.
>> > > So in this patch we do the same thing but for RX side. We add
>> > > a sk_psock_strp_enabled() helper so TLS can learn a BPF verdict
>> > > program is running and add a tls_sw_has_ctx_rx() helper so BPF
>> > > side can learn there is a TLS ULP on the socket.
>> > >
>> > > Then on BPF side we omit calling our stream parser to avoid
>> > > breaking the data stream for the KTLS receiver. Then on the
>> > > KTLS side we call BPF_SK_SKB_STREAM_VERDICT once the KTLS
>> > > receiver is done with the packet but before it posts the
>> > > msg to userspace. This gives us symmetry between the TX and
>> > > RX halfs and IMO makes it usable again. On the TX side we
>> > > process packets in this order BPF -> TLS -> TCP and on
>> > > the receive side in the reverse order TCP -> TLS -> BPF.
>> > >
>> > > Discovered while testing OpenSSL 3.0 Alpha2.0 release.
>> > >
>> > > Fixes: d829e9c4112b5 ("tls: convert to generic sk_msg interface")
>> > > Signed-off-by: John Fastabend <john.fastabend@...il.com>
>> > > ---
>
> [...]
>
>> > > +static void sk_psock_tls_verdict_apply(struct sk_psock *psock,
>> > > +				       struct sk_buff *skb, int verdict)
>> > > +{
>> > > +	switch (verdict) {
>> > > +	case __SK_REDIRECT:
>> > > +		sk_psock_skb_redirect(psock, skb);
>> > > +		break;
>> > > +	case __SK_PASS:
>> > > +	case __SK_DROP:
>> >
>> > The two cases above need a "fallthrough;", right?
>>
>> Correct otherwise will get the "fallthrough" patch shortly after this
>> lands. Thanks I'll add it.
>>
>
> hmm actually I don't think we need 'fallthrough;' here when the
> case doesn't have statements,
>
>  switch (a) {
>  case 1:
>  case 2:
>  default:
>      break;
>  }
>
> seems OK to me. I don't have a preference though so feel free to
> correct me.

I misunderstood guidance in [0]. You're right, it seems too verbose to
annotate cases without statements. Didn't mean to nit-pick :-)

[0] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through