[PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Jakub Kicinski]

TLS expects that it owns the receive queue of the TCP socket.
This cannot be guaranteed in case the reader of the TCP socket
entered before the TLS ULP was installed, or uses some non-standard
read API (eg. zerocopy ones). Make sure that the TCP sequence
numbers match between ->data_ready and ->recvmsg, otherwise
don't trust the work that ->data_ready has done.

Signed-off-by: William Liu <will@willsroot.io>
Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>
Link: https://lore.kernel.org/tFjq_kf7sWIG3A7CrCg_egb8CVsT_gsmHAK0_wxDPJXfIzxFAMxqmLwp3MlU5EHiet0AwwJldaaFdgyHpeIUCS-3m3llsmRzp9xIOBR4lAI=@syst3mfailure.io
Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/tls.h  |  1 +
 net/tls/tls.h      |  2 +-
 net/tls/tls_strp.c | 17 ++++++++++++++---
 net/tls/tls_sw.c   |  3 ++-
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 857340338b69..37344a39e4c9 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -117,6 +117,7 @@ struct tls_strparser {
 	bool msg_ready;
 
 	struct strp_msg stm;
+	u32 copied_seq;
 
 	struct sk_buff *anchor;
 	struct work_struct work;
diff --git a/net/tls/tls.h b/net/tls/tls.h
index 774859b63f0d..4e077068e6d9 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -196,7 +196,7 @@ void tls_strp_msg_done(struct tls_strparser *strp);
 int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb);
 void tls_rx_msg_ready(struct tls_strparser *strp);
 
-void tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh);
+bool tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh);
 int tls_strp_msg_cow(struct tls_sw_context_rx *ctx);
 struct sk_buff *tls_strp_msg_detach(struct tls_sw_context_rx *ctx);
 int tls_strp_msg_hold(struct tls_strparser *strp, struct sk_buff_head *dst);
diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
index 095cf31bae0b..4bac58174cc3 100644
--- a/net/tls/tls_strp.c
+++ b/net/tls/tls_strp.c
@@ -473,9 +473,11 @@ static void tls_strp_load_anchor_with_queue(struct tls_strparser *strp, int len)
 	strp->anchor->destructor = NULL;
 
 	strp->stm.offset = offset;
+
+	strp->copied_seq = tp->copied_seq;
 }
 
-void tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh)
+bool tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh)
 {
 	struct strp_msg *rxm;
 	struct tls_msg *tlm;
@@ -484,8 +486,15 @@ void tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh)
 	DEBUG_NET_WARN_ON_ONCE(!strp->stm.full_len);
 
 	if (!strp->copy_mode && force_refresh) {
-		if (WARN_ON(tcp_inq(strp->sk) < strp->stm.full_len))
-			return;
+		struct tcp_sock *tp = tcp_sk(strp->sk);
+
+		if (unlikely(strp->copied_seq != tp->copied_seq ||
+			     WARN_ON(tcp_inq(strp->sk) < strp->stm.full_len))) {
+
+			WRITE_ONCE(strp->msg_ready, 0);
+			memset(&strp->stm, 0, sizeof(strp->stm));
+			return false;
+		}
 
 		tls_strp_load_anchor_with_queue(strp, strp->stm.full_len);
 	}
@@ -495,6 +504,8 @@ void tls_strp_msg_load(struct tls_strparser *strp, bool force_refresh)
 	rxm->offset	= strp->stm.offset;
 	tlm = tls_msg(strp->anchor);
 	tlm->control	= strp->mark;
+
+	return true;
 }
 
 /* Called with lock held on lower socket */
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 549d1ea01a72..51c98a007dda 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1384,7 +1384,8 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 			return sock_intr_errno(timeo);
 	}
 
-	tls_strp_msg_load(&ctx->strp, released);
+	if (unlikely(!tls_strp_msg_load(&ctx->strp, released)))
+		return tls_rx_rec_wait(sk, psock, nonblock, false);
 
 	return 1;
 }
-- 
2.50.1

[PATCH net 2/2] selftests: tls: test TCP stealing data from under the TLS socket

[Jakub Kicinski]

Check a race where data disappears from the TCP socket after
TLS signaled that its ready to receive.

  ok 6 global.data_steal
  #  RUN           tls_basic.base_base ...
  #            OK  tls_basic.base_base

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/tls.c | 63 +++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 5ded3b3a7538..d8cfcf9bb825 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2708,6 +2708,69 @@ TEST(prequeue) {
 	close(cfd);
 }
 
+TEST(data_steal) {
+	struct tls_crypto_info_keys tls;
+	char buf[20000], buf2[20000];
+	struct sockaddr_in addr;
+	int sfd, cfd, ret, fd;
+	int pid, status;
+	socklen_t len;
+
+	len = sizeof(addr);
+	memrnd(buf, sizeof(buf));
+
+	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_GCM_256, &tls, 0);
+
+	addr.sin_family = AF_INET;
+	addr.sin_addr.s_addr = htonl(INADDR_ANY);
+	addr.sin_port = 0;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	sfd = socket(AF_INET, SOCK_STREAM, 0);
+
+	ASSERT_EQ(bind(sfd, &addr, sizeof(addr)), 0);
+	ASSERT_EQ(listen(sfd, 10), 0);
+	ASSERT_EQ(getsockname(sfd, &addr, &len), 0);
+	ASSERT_EQ(connect(fd, &addr, sizeof(addr)), 0);
+	ASSERT_GE(cfd = accept(sfd, &addr, &len), 0);
+	close(sfd);
+
+	ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
+	if (ret) {
+		ASSERT_EQ(errno, ENOENT);
+		SKIP(return, "no TLS support");
+	}
+	ASSERT_EQ(setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls")), 0);
+
+	/* Spawn a child and get it into the read wait path of the underlying
+	 * TCP socket.
+	 */
+	pid = fork();
+	ASSERT_GE(pid, 0);
+	if (!pid) {
+		EXPECT_EQ(recv(cfd, buf, sizeof(buf), MSG_WAITALL),
+			  sizeof(buf));
+		exit(!__test_passed(_metadata));
+	}
+
+	usleep(2000);
+	ASSERT_EQ(setsockopt(fd, SOL_TLS, TLS_TX, &tls, tls.len), 0);
+	ASSERT_EQ(setsockopt(cfd, SOL_TLS, TLS_RX, &tls, tls.len), 0);
+
+	EXPECT_EQ(send(fd, buf, sizeof(buf), 0), sizeof(buf));
+	usleep(2000);
+	EXPECT_EQ(recv(cfd, buf2, sizeof(buf2), MSG_DONTWAIT), -1);
+	/* Don't check errno, the error will be different depending
+	 * on what random bytes TLS interpreted as the record length.
+	 */
+
+	close(fd);
+	close(cfd);
+
+	EXPECT_EQ(wait(&status), pid);
+	EXPECT_EQ(status, 0);
+}
+
 static void __attribute__((constructor)) fips_check(void) {
 	int res;
 	FILE *f;
-- 
2.50.1

Re: [PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Eric Dumazet]

On Wed, Aug 6, 2025 at 11:05 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> TLS expects that it owns the receive queue of the TCP socket.
> This cannot be guaranteed in case the reader of the TCP socket
> entered before the TLS ULP was installed, or uses some non-standard
> read API (eg. zerocopy ones). Make sure that the TCP sequence
> numbers match between ->data_ready and ->recvmsg, otherwise
> don't trust the work that ->data_ready has done.
>
> Signed-off-by: William Liu <will@willsroot.io>
> Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>

I presume you meant Reported-by tags ?

> Link: https://lore.kernel.org/tFjq_kf7sWIG3A7CrCg_egb8CVsT_gsmHAK0_wxDPJXfIzxFAMxqmLwp3MlU5EHiet0AwwJldaaFdgyHpeIUCS-3m3llsmRzp9xIOBR4lAI=@syst3mfailure.io
> Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/net/tls.h  |  1 +
>  net/tls/tls.h      |  2 +-
>  net/tls/tls_strp.c | 17 ++++++++++++++---
>  net/tls/tls_sw.c   |  3 ++-
>  4 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 857340338b69..37344a39e4c9 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -117,6 +117,7 @@ struct tls_strparser {
>         bool msg_ready;
>
>         struct strp_msg stm;
> +       u32 copied_seq;

Can a 2^32 wrap occur eventually ?

Re: [PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Jakub Kicinski]

On Wed, 6 Aug 2025 11:35:28 -0700 Eric Dumazet wrote:
> > TLS expects that it owns the receive queue of the TCP socket.
> > This cannot be guaranteed in case the reader of the TCP socket
> > entered before the TLS ULP was installed, or uses some non-standard
> > read API (eg. zerocopy ones). Make sure that the TCP sequence
> > numbers match between ->data_ready and ->recvmsg, otherwise
> > don't trust the work that ->data_ready has done.
> >
> > Signed-off-by: William Liu <will@willsroot.io>
> > Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>  
> 
> I presume you meant Reported-by tags ?

Oops..

> > Link: https://lore.kernel.org/tFjq_kf7sWIG3A7CrCg_egb8CVsT_gsmHAK0_wxDPJXfIzxFAMxqmLwp3MlU5EHiet0AwwJldaaFdgyHpeIUCS-3m3llsmRzp9xIOBR4lAI=@syst3mfailure.io
> > Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  include/net/tls.h  |  1 +
> >  net/tls/tls.h      |  2 +-
> >  net/tls/tls_strp.c | 17 ++++++++++++++---
> >  net/tls/tls_sw.c   |  3 ++-
> >  4 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/tls.h b/include/net/tls.h
> > index 857340338b69..37344a39e4c9 100644
> > --- a/include/net/tls.h
> > +++ b/include/net/tls.h
> > @@ -117,6 +117,7 @@ struct tls_strparser {
> >         bool msg_ready;
> >
> >         struct strp_msg stm;
> > +       u32 copied_seq;  
> 
> Can a 2^32 wrap occur eventually ?

Hm, good point. Is it good enough if we also check it in data_ready?
That way we should notice that someone is eating our data before
the seq had a chance to wrap?

Re: [PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Eric Dumazet]

On Wed, Aug 6, 2025 at 1:20 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 6 Aug 2025 11:35:28 -0700 Eric Dumazet wrote:
> > > TLS expects that it owns the receive queue of the TCP socket.
> > > This cannot be guaranteed in case the reader of the TCP socket
> > > entered before the TLS ULP was installed, or uses some non-standard
> > > read API (eg. zerocopy ones). Make sure that the TCP sequence
> > > numbers match between ->data_ready and ->recvmsg, otherwise
> > > don't trust the work that ->data_ready has done.
> > >
> > > Signed-off-by: William Liu <will@willsroot.io>
> > > Signed-off-by: Savino Dicanosa <savy@syst3mfailure.io>
> >
> > I presume you meant Reported-by tags ?
>
> Oops..
>
> > > Link: https://lore.kernel.org/tFjq_kf7sWIG3A7CrCg_egb8CVsT_gsmHAK0_wxDPJXfIzxFAMxqmLwp3MlU5EHiet0AwwJldaaFdgyHpeIUCS-3m3llsmRzp9xIOBR4lAI=@syst3mfailure.io
> > > Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > > ---
> > >  include/net/tls.h  |  1 +
> > >  net/tls/tls.h      |  2 +-
> > >  net/tls/tls_strp.c | 17 ++++++++++++++---
> > >  net/tls/tls_sw.c   |  3 ++-
> > >  4 files changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/net/tls.h b/include/net/tls.h
> > > index 857340338b69..37344a39e4c9 100644
> > > --- a/include/net/tls.h
> > > +++ b/include/net/tls.h
> > > @@ -117,6 +117,7 @@ struct tls_strparser {
> > >         bool msg_ready;
> > >
> > >         struct strp_msg stm;
> > > +       u32 copied_seq;
> >
> > Can a 2^32 wrap occur eventually ?
>
> Hm, good point. Is it good enough if we also check it in data_ready?
> That way we should notice that someone is eating our data before
> the seq had a chance to wrap?

I could not understand what your suggestion was.

Perhaps store both copued_seq and tp->bytes_received and

check if (tp->bytes_received - strp->bytes_received) is smaller than 2^31 .

              if (unlikely(strp->copied_seq != tp->copied_seq ||
                               (tp->bytes_received -
strp->bytes_received >= (1ULL < 31)) ||
                            WARN_ON(tcp_inq(strp->sk) < strp->stm.full_len))) {

Re: [PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Jakub Kicinski]

On Fri, 8 Aug 2025 06:51:06 -0700 Eric Dumazet wrote:
> > > Can a 2^32 wrap occur eventually ?  
> >
> > Hm, good point. Is it good enough if we also check it in data_ready?
> > That way we should notice that someone is eating our data before
> > the seq had a chance to wrap?  
> 
> I could not understand what your suggestion was.
> 
> Perhaps store both copued_seq and tp->bytes_received and
> 
> check if (tp->bytes_received - strp->bytes_received) is smaller than 2^31 .
> 
>               if (unlikely(strp->copied_seq != tp->copied_seq ||
>                                (tp->bytes_received -
> strp->bytes_received >= (1ULL < 31)) ||
>                             WARN_ON(tcp_inq(strp->sk) < strp->stm.full_len))) {

Nice, I think that would work.
I was wondering how to solve this yesterday and I realized the extra
condition isn't really needed. We just have to handle the inq < full_len
more carefully and remove the WARN_ON(). I posted a v2.

Re: [PATCH net 1/2] tls: handle data disappearing from under the TLS ULP

[Eric Dumazet]

On Fri, Aug 8, 2025 at 6:57 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 8 Aug 2025 06:51:06 -0700 Eric Dumazet wrote:
> > > > Can a 2^32 wrap occur eventually ?
> > >
> > > Hm, good point. Is it good enough if we also check it in data_ready?
> > > That way we should notice that someone is eating our data before
> > > the seq had a chance to wrap?
> >
> > I could not understand what your suggestion was.
> >
> > Perhaps store both copued_seq and tp->bytes_received and
> >
> > check if (tp->bytes_received - strp->bytes_received) is smaller than 2^31 .
> >
> >               if (unlikely(strp->copied_seq != tp->copied_seq ||
> >                                (tp->bytes_received -
> > strp->bytes_received >= (1ULL < 31)) ||
> >                             WARN_ON(tcp_inq(strp->sk) < strp->stm.full_len))) {
>
> Nice, I think that would work.
> I was wondering how to solve this yesterday and I realized the extra
> condition isn't really needed. We just have to handle the inq < full_len
> more carefully and remove the WARN_ON(). I posted a v2.

Ah I must have missed the v2, let me check it.