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
| ||
|
Message-ID: <42d66dde-29d6-f948-bc2e-72465beb800f@linux.dev> Date: Mon, 23 Oct 2023 14:35:47 -0700 From: Martin KaFai Lau <martin.lau@...ux.dev> To: Kuniyuki Iwashima <kuniyu@...zon.com> Cc: andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org, daniel@...earbox.net, davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, haoluo@...gle.com, john.fastabend@...il.com, jolsa@...nel.org, kpsingh@...nel.org, kuba@...nel.org, kuni1840@...il.com, mykolal@...com, netdev@...r.kernel.org, pabeni@...hat.com, sdf@...gle.com, sinquersw@...il.com, song@...nel.org, yonghong.song@...ux.dev Subject: Re: [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks. On 10/20/23 11:48 PM, Kuniyuki Iwashima wrote: > I think this was doable. With the diff below, I was able to skip > validation in cookie_v[46]_check() when if skb->sk is not NULL. > > The kfunc allocates req and set req->syncookie to 1, which is usually > set in TX path, so if it's 1 in RX (inet_steal_sock()), we can see > that req is allocated by kfunc (at least, req->syncookie && > req->rsk_listener never be true in the current TCP stack). > > The difference here is that req allocated by kfunc holds refcnt of > rsk_listener (passing true to inet_reqsk_alloc()) to prevent freeing > the listener until req reaches cookie_v[46]_check(). The cookie_v[46]_check() holds the listener sk refcnt now? > > The cookie generation at least should be done at tc/xdp. The > valdation can be done earlier as well on tc/xdp, but it could > add another complexity, listener's life cycle if we allocate > req there. I think your code below looks pretty close already. It seems the only concern/complexity is the extra rsk_listener refcnt (btw the concern is on performance for the extra refcnt? or there is correctness issue?). Asking because bpf_sk_assign() can already assign a listener to skb->sk and it also does not take a refcnt on the listener. The same no refcnt needed on req->rsk_listener should be doable also. sock_pfree may need to be smarter to check req->syncookie. What else may need to change? > > I'm wondering which place to add the validation capability, and > I think SOCK_OPS is simpler than tc. > > #1 validate cookie and allocate req at tc, and skip validation > > #2 validate cookie (and update bpf map at xdp/tc, and look up bpf > map) and allocate req at SOCK_OPS hook > > Given SYN proxy is usually on the other node and incoming cookie > is almost always valid, we might need not validate it in the early > stage in the stack. > > What do you think ? Yeah, supporting validation in sock_ops is an open option if the tc side is too hard but I feel you are pretty close on the tc side. > > ---8<--- > diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h > index 3ecfeadbfa06..e5e4627bf270 100644 > --- a/include/net/inet_hashtables.h > +++ b/include/net/inet_hashtables.h > @@ -462,9 +462,19 @@ struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff, > if (!sk) > return NULL; > > - if (!prefetched || !sk_fullsock(sk)) > + if (!prefetched) > return sk; > > + if (!sk_fullsock(sk)) { > + if (sk->sk_state == TCP_NEW_SYN_RECV && inet_reqsk(sk)->syncookie) { > + skb->sk = sk; > + skb->destructor = sock_pfree; > + sk = inet_reqsk(sk)->rsk_listener; > + } > + > + return sk; > + } > + > if (sk->sk_protocol == IPPROTO_TCP) { > if (sk->sk_state != TCP_LISTEN) > return sk; > diff --git a/net/core/filter.c b/net/core/filter.c > index cc2e4babc85f..bca491ddf42c 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -11800,6 +11800,71 @@ __bpf_kfunc int bpf_sock_addr_set_sun_path(struct bpf_sock_addr_kern *sa_kern, > > return 0; > } > + > +__bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct sk_buff *skb, struct sock *sk, > + struct tcp_options_received *tcp_opt, > + int tcp_opt__sz, u16 mss) > +{ > + const struct tcp_request_sock_ops *af_ops; > + const struct request_sock_ops *ops; > + struct inet_request_sock *ireq; > + struct tcp_request_sock *treq; > + struct request_sock *req; > + > + if (!sk) > + return -EINVAL; > + > + if (!skb_at_tc_ingress(skb)) > + return -EINVAL; > + > + if (dev_net(skb->dev) != sock_net(sk)) > + return -ENETUNREACH; > + > + switch (sk->sk_family) { > + case AF_INET: /* TODO: MPTCP */ > + ops = &tcp_request_sock_ops; > + af_ops = &tcp_request_sock_ipv4_ops; > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + ops = &tcp6_request_sock_ops; > + af_ops = &tcp_request_sock_ipv6_ops; > + break; > +#endif > + default: > + return -EINVAL; > + } > + > + if (sk->sk_type != SOCK_STREAM || sk->sk_state != TCP_LISTEN) > + return -EINVAL; > + > + req = inet_reqsk_alloc(ops, sk, true); > + if (!req) > + return -ENOMEM; > + > + ireq = inet_rsk(req); > + treq = tcp_rsk(req); > + > + refcount_set(&req->rsk_refcnt, 1); > + req->syncookie = 1; > + req->mss = mss; > + req->ts_recent = tcp_opt->saw_tstamp ? tcp_opt->rcv_tsval : 0; > + > + ireq->snd_wscale = tcp_opt->snd_wscale; > + ireq->sack_ok = tcp_opt->sack_ok; > + ireq->wscale_ok = tcp_opt->wscale_ok; > + ireq->tstamp_ok = tcp_opt->saw_tstamp; > + > + tcp_rsk(req)->af_specific = af_ops; > + tcp_rsk(req)->ts_off = tcp_opt->rcv_tsecr - tcp_ns_to_ts(tcp_clock_ns()); > + > + skb_orphan(skb); > + skb->sk = req_to_sk(req); > + skb->destructor = sock_pfree; > + > + return 0; > +} > + > __diag_pop(); > > int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags, > @@ -11828,6 +11893,10 @@ BTF_SET8_START(bpf_kfunc_check_set_sock_addr) > BTF_ID_FLAGS(func, bpf_sock_addr_set_sun_path) > BTF_SET8_END(bpf_kfunc_check_set_sock_addr) > > +BTF_SET8_START(bpf_kfunc_check_set_tcp_reqsk) > +BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk) > +BTF_SET8_END(bpf_kfunc_check_set_tcp_reqsk) > + > static const struct btf_kfunc_id_set bpf_kfunc_set_skb = { > .owner = THIS_MODULE, > .set = &bpf_kfunc_check_set_skb, > @@ -11843,6 +11912,11 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_sock_addr = { > .set = &bpf_kfunc_check_set_sock_addr, > }; > > +static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = { > + .owner = THIS_MODULE, > + .set = &bpf_kfunc_check_set_tcp_reqsk, > +}; > + > static int __init bpf_kfunc_init(void) > { > int ret; > @@ -11858,8 +11932,10 @@ static int __init bpf_kfunc_init(void) > ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb); > ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb); > ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp); > - return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, > - &bpf_kfunc_set_sock_addr); > + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, > + &bpf_kfunc_set_sock_addr); > + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk); > + return ret; > } > late_initcall(bpf_kfunc_init); > > ---8<---
Powered by blists - more mailing lists