netdev - Re: [bpf-next PATCH 1/2] bpf, sockmap: add skb_adjust

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] [day] [month] [year] [list]

Message-ID: <5f7537e9802fb_364f82081e@john-XPS-13-9370.notmuch>
Date:   Wed, 30 Sep 2020 18:59:05 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        lmb@...udflare.com, Marek Majkowski <marek@...udflare.com>
Subject: Re: [bpf-next PATCH 1/2] bpf, sockmap: add skb_adjust_room to pop
 bytes off ingress payload

Jakub Sitnicki wrote:
> On Sat, Sep 26, 2020 at 06:27 AM CEST, John Fastabend wrote:
> > This implements a new helper skb_adjust_room() so users can push/pop
> > extra bytes from a BPF_SK_SKB_STREAM_VERDICT program.
> >
> > Some protocols may include headers and other information that we may
> > not want to include when doing a redirect from a BPF_SK_SKB_STREAM_VERDICT
> > program. One use case is to redirect TLS packets into a receive socket
> > that doesn't expect TLS data. In TLS case the first 13B or so contain the
> > protocol header. With KTLS the payload is decrypted so we should be able
> > to redirect this to a receiving socket, but the receiving socket may not
> > be expecting to receive a TLS header and discard the data. Using the
> > above helper we can pop the header off and put an appropriate header on
> > the payload. This allows for creating a proxy between protocols without
> > extra hops through the stack or userspace.
> 
> This is useful stuff. Apart from the TLS use-case, you might want to pop
> off proxy headers like PROXY v1/v2 (CC Marek):
> 
>   https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

Great!

> 
> >
> > So in order to fix this case add skb_adjust_room() so users can strip the
> > header. After this the user can strip the header and an unmodified receiver
> > thread will work correctly when data is redirected into the ingress path
> > of a sock.
> >
> > Signed-off-by: John Fastabend <john.fastabend@...il.com>
> > ---
> >  net/core/filter.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 4d8dc7a31a78..d232358f1dcd 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -76,6 +76,7 @@
> >  #include <net/bpf_sk_storage.h>
> >  #include <net/transp_v6.h>
> >  #include <linux/btf_ids.h>
> > +#include <net/tls.h>
> >
> >  static const struct bpf_func_proto *
> >  bpf_sk_base_func_proto(enum bpf_func_id func_id);
> > @@ -3218,6 +3219,53 @@ static u32 __bpf_skb_max_len(const struct sk_buff *skb)
> >  			  SKB_MAX_ALLOC;
> >  }
> >
> > +BPF_CALL_4(sk_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
> > +	   u32, mode, u64, flags)
> > +{
> > +	unsigned int len_diff_abs = abs(len_diff);
> > +	bool shrink = len_diff < 0;
> > +	int ret = 0;
> > +
> > +	if (unlikely(flags))
> > +		return -EINVAL;
> > +	if (unlikely(len_diff_abs > 0xfffU))
> > +		return -EFAULT;
> > +
> > +	if (!shrink) {
> > +		unsigned int grow = len_diff;
> > +
> > +		ret = skb_cow(skb, grow);
> > +		if (likely(!ret)) {
> > +			__skb_push(skb, len_diff_abs);
> > +			memset(skb->data, 0, len_diff_abs);
> > +		}
> > +	} else {
> > +		/* skb_ensure_writable() is not needed here, as we're
> > +		 * already working on an uncloned skb.
> > +		 */
> 
> I'm trying to digest the above comment. What if:

I'll delete the comment its not accurate. We happily write headers
from verdict programs today. Do you have a specific concern or
just noticing I was a bit careless and cut'n'pasted an incorrect
comment around.

> 
> static int __strp_recv(…)
> {
>         …
> 	while (eaten < orig_len) {
> 		/* Always clone since we will consume something */
> 		skb = skb_clone(orig_skb, GFP_ATOMIC);
>                 …
> 		head = strp->skb_head;
> 		if (!head) {
> 			head = skb;
>                         …
> 		} else {
>                         …
> 		}
>                 …
> 		/* Give skb to upper layer */
> 		strp->cb.rcv_msg(strp, head); // → sk_psock_init_strp
>                 …
> 	}
>         …
> }
> 
> That looks like a code path where we pass a cloned SKB.

Right but its there to cover the sk_eat_skb in tcp_read_sock()
otherwise

 sk_eat_skb() -> __kfree_skb() -> skb_release_all()

would go all the way to page_frag_free().

> 
> > +		if (unlikely(!pskb_may_pull(skb, len_diff_abs)))
> > +			return -ENOMEM;
> > +		__skb_pull(skb, len_diff_abs);
> > +	}
> > +	bpf_compute_data_end_sk_skb(skb);
> > +	if (tls_sw_has_ctx_rx(skb->sk)) {
> > +		struct strp_msg *rxm = strp_msg(skb);
> > +
> > +		rxm->full_len += len_diff;
> > +	}
> > +	return ret;
> > +}
> 
> [...]