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
| ||
|
Date: Wed, 20 Jul 2022 10:37:02 +0200 From: Eric Dumazet <edumazet@...gle.com> To: Jakub Kicinski <kuba@...nel.org> Cc: David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>, Boris Pismenny <borisp@...dia.com>, John Fastabend <john.fastabend@...il.com>, Maxim Mikityanskiy <maximmi@...dia.com>, Tariq Toukan <tariqt@...dia.com>, vfedorenko@...ek.ru Subject: Re: [PATCH net-next v2 01/11] tls: rx: allow only one reader at a time On Fri, Jul 15, 2022 at 7:22 AM Jakub Kicinski <kuba@...nel.org> wrote: > > recvmsg() in TLS gets data from the skb list (rx_list) or fresh > skbs we read from TCP via strparser. The former holds skbs which were > already decrypted for peek or decrypted and partially consumed. > > tls_wait_data() only notices appearance of fresh skbs coming out > of TCP (or psock). It is possible, if there is a concurrent call > to peek() and recv() that the peek() will move the data from input > to rx_list without recv() noticing. recv() will then read data out > of order or never wake up. > > This is not a practical use case/concern, but it makes the self > tests less reliable. This patch solves the problem by allowing > only one reader in. > > Because having multiple processes calling read()/peek() is not > normal avoid adding a lock and try to fast-path the single reader > case. > > Signed-off-by: Jakub Kicinski <kuba@...nel.org> > --- > include/net/tls.h | 3 +++ > net/tls/tls_sw.c | 61 +++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 57 insertions(+), 7 deletions(-) > > diff --git a/include/net/tls.h b/include/net/tls.h > index 8742e13bc362..e8935cfe0cd6 100644 > --- a/include/net/tls.h > +++ b/include/net/tls.h > @@ -116,11 +116,14 @@ struct tls_sw_context_rx { > void (*saved_data_ready)(struct sock *sk); > > struct sk_buff *recv_pkt; > + u8 reader_present; > u8 async_capable:1; > u8 zc_capable:1; > + u8 reader_contended:1; > atomic_t decrypt_pending; > /* protect crypto_wait with decrypt_pending*/ > spinlock_t decrypt_compl_lock; > + struct wait_queue_head wq; > }; > > struct tls_record_info { > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 68d79ee48a56..761a63751616 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1753,6 +1753,51 @@ tls_read_flush_backlog(struct sock *sk, struct tls_prot_info *prot, > sk_flush_backlog(sk); > } > > +static long tls_rx_reader_lock(struct sock *sk, struct tls_sw_context_rx *ctx, > + bool nonblock) > +{ > + long timeo; > + > + lock_sock(sk); > + > + timeo = sock_rcvtimeo(sk, nonblock); > + > + while (unlikely(ctx->reader_present)) { > + DEFINE_WAIT_FUNC(wait, woken_wake_function); > + > + ctx->reader_contended = 1; > + > + add_wait_queue(&ctx->wq, &wait); > + sk_wait_event(sk, &timeo, > + !READ_ONCE(ctx->reader_present), &wait); > + remove_wait_queue(&ctx->wq, &wait); > + > + if (!timeo) > + return -EAGAIN; We return with socket lock held, and callers seem to not release the lock. > + if (signal_pending(current)) > + return sock_intr_errno(timeo); same here. Let's wait for syzbot to catch up :) > + } > + > + WRITE_ONCE(ctx->reader_present, 1); > + > + return timeo; > +} > + > +static void tls_rx_reader_unlock(struct sock *sk, struct tls_sw_context_rx *ctx) > +{ > + if (unlikely(ctx->reader_contended)) { > + if (wq_has_sleeper(&ctx->wq)) > + wake_up(&ctx->wq); > + else > + ctx->reader_contended = 0; > + > + WARN_ON_ONCE(!ctx->reader_present); > + } > + > + WRITE_ONCE(ctx->reader_present, 0); > + release_sock(sk); > +} > + > int tls_sw_recvmsg(struct sock *sk, > struct msghdr *msg, > size_t len, > @@ -1782,7 +1827,9 @@ int tls_sw_recvmsg(struct sock *sk, > return sock_recv_errqueue(sk, msg, len, SOL_IP, IP_RECVERR); > > psock = sk_psock_get(sk); > - lock_sock(sk); > + timeo = tls_rx_reader_lock(sk, ctx, flags & MSG_DONTWAIT); > + if (timeo < 0) > + return timeo; > bpf_strp_enabled = sk_psock_strp_enabled(psock); > > /* If crypto failed the connection is broken */ > @@ -1801,7 +1848,6 @@ int tls_sw_recvmsg(struct sock *sk, > > target = sock_rcvlowat(sk, flags & MSG_WAITALL, len); > len = len - copied; > - timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT); > > zc_capable = !bpf_strp_enabled && !is_kvec && !is_peek && > ctx->zc_capable; > @@ -1956,7 +2002,7 @@ int tls_sw_recvmsg(struct sock *sk, > copied += decrypted; > > end: > - release_sock(sk); > + tls_rx_reader_unlock(sk, ctx); > if (psock) > sk_psock_put(sk, psock); > return copied ? : err; > @@ -1978,9 +2024,9 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, > long timeo; > int chunk; > > - lock_sock(sk); > - > - timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK); > + timeo = tls_rx_reader_lock(sk, ctx, flags & SPLICE_F_NONBLOCK); > + if (timeo < 0) > + return timeo; > > from_queue = !skb_queue_empty(&ctx->rx_list); > if (from_queue) { > @@ -2029,7 +2075,7 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos, > } > > splice_read_end: > - release_sock(sk); > + tls_rx_reader_unlock(sk, ctx); > return copied ? : err; > } > > @@ -2371,6 +2417,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx) > } else { > crypto_init_wait(&sw_ctx_rx->async_wait); > spin_lock_init(&sw_ctx_rx->decrypt_compl_lock); > + init_waitqueue_head(&sw_ctx_rx->wq); > crypto_info = &ctx->crypto_recv.info; > cctx = &ctx->rx; > skb_queue_head_init(&sw_ctx_rx->rx_list); > -- > 2.36.1 >
Powered by blists - more mailing lists