netdev - Re: [PATCH v4 bpf-next 1/3] bpf: tcp: Handle BPF SYN Cookie in cookie_v[46]

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: Wed, 6 Dec 2023 10:29:52 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <martin.lau@...ux.dev>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
	<daniel@...earbox.net>, <edumazet@...gle.com>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 bpf-next 1/3] bpf: tcp: Handle BPF SYN Cookie in cookie_v[46]_check().

From: Martin KaFai Lau <martin.lau@...ux.dev>
Date: Tue, 5 Dec 2023 16:19:20 -0800
> On 12/4/23 5:34 PM, Kuniyuki Iwashima wrote:
> > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > index 61f1c96cfe63..0f9c3aed2014 100644
> > --- a/net/ipv4/syncookies.c
> > +++ b/net/ipv4/syncookies.c
> > @@ -304,6 +304,59 @@ static int cookie_tcp_reqsk_init(struct sock *sk, struct sk_buff *skb,
> >   	return 0;
> >   }
> >   
> > +#if IS_ENABLED(CONFIG_BPF)
> > +struct request_sock *cookie_bpf_check(struct net *net, struct sock *sk,
> > +				      struct sk_buff *skb)
> > +{
> > +	struct request_sock *req = inet_reqsk(skb->sk);
> > +	struct inet_request_sock *ireq = inet_rsk(req);
> > +	struct tcp_request_sock *treq = tcp_rsk(req);
> > +	struct tcp_options_received tcp_opt;
> > +	int ret;
> > +
> > +	skb->sk = NULL;
> > +	skb->destructor = NULL;
> > +	req->rsk_listener = NULL;
> > +
> > +	memset(&tcp_opt, 0, sizeof(tcp_opt));
> > +	tcp_parse_options(net, skb, &tcp_opt, 0, NULL);
> 
> In patch 2, the bpf prog is passing the tcp_opt to the kfunc. The selftest in 
> patch 3 is also parsing the tcp-options.
> 
> The kernel parses the tcp-option here again to do some checking and req's member 
> initialization. Can these checking and initialization be done in the 
> bpf_sk_assign_tcp_reqsk() kfunc instead to avoid the double tcp-option parsing?

If TS is not used as a cookie storage, bpf prog need not parse it.
OTOH, if a value is encoded into TS, bpf prog need to parse it.
In that case, we cannot avoid parsing options in bpf prog.

The parsing here comes from my paranoia, so.. probably we can drop it
and the first test below, and rely on bpf prog's tcp_opt, especially
tstamp_ok, rcv_tsval, and rcv_tsecr ?

I placed other tests here to align with the normal cookie flow, but
they can be moved to kfunc.  However, initialisation assuems skb
points to TCP header, so here would be better place, I think.


> 
> > +
> > +	if (ireq->tstamp_ok ^ tcp_opt.saw_tstamp) {
> > +		__NET_INC_STATS(net, LINUX_MIB_SYNCOOKIESFAILED);
> > +		goto reset;
> > +	}
> > +
> > +	__NET_INC_STATS(net, LINUX_MIB_SYNCOOKIESRECV);
> > +
> > +	if (ireq->tstamp_ok) {
> > +		if (!READ_ONCE(net->ipv4.sysctl_tcp_timestamps))
> > +			goto reset;
> > +
> > +		req->ts_recent = tcp_opt.rcv_tsval;
> > +		treq->ts_off = tcp_opt.rcv_tsecr - tcp_ns_to_ts(false, tcp_clock_ns());
> > +	}
> > +
> > +	if (ireq->sack_ok && !READ_ONCE(net->ipv4.sysctl_tcp_sack))
> > +		goto reset;
> > +
> > +	if (ireq->wscale_ok && !READ_ONCE(net->ipv4.sysctl_tcp_window_scaling))
> > +		goto reset;
> > +
> > +	ret = cookie_tcp_reqsk_init(sk, skb, req);
> > +	if (ret) {
> > +		reqsk_free(req);
> > +		req = NULL;
> > +	}
> > +
> > +	return req;
> > +
> > +reset:
> > +	reqsk_free(req);
> > +	return ERR_PTR(-EINVAL);
> > +}
> > +EXPORT_SYMBOL_GPL(cookie_bpf_check);
> > +#endif