[PATCH net-next v2 1/2] tls: read sk_err once in tls_encrypt_done()

[PATCH net-next v2 1/2] tls: read sk_err once in tls_encrypt_done()

[Rishikesh Jethwani]

tls_encrypt_done() runs from the crypto completion, without the socket
lock, and reads sk->sk_err twice. getsockopt(SO_ERROR) and sock_error()
clear sk_err locklessly, so it can go from non-zero to zero between the
two reads. A successful completion (err == 0) then takes the "no error
set on the socket" branch and calls tls_err_abort(sk, 0), which hits
WARN_ON_ONCE(err >= 0) and clears sk_err.

Read sk_err once and use that value for both checks.

Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
Reported-by: syzbot+cca46a9d1276f38af2ae@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cca46a9d1276f38af2ae
Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 net/tls/tls_sw.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 771e02bff519..b4535139438a 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -473,6 +473,7 @@ static void tls_encrypt_done(void *data, int err)
 	struct scatterlist *sge;
 	struct sk_msg *msg_en;
 	struct sock *sk;
+	int sk_err;
 
 	if (err == -EINPROGRESS) /* see the comment in tls_decrypt_done() */
 		return;
@@ -489,12 +490,13 @@ static void tls_encrypt_done(void *data, int err)
 	sge->length += prot->prepend_size;
 
 	/* Check if error is previously set on socket */
-	if (err || sk->sk_err) {
+	sk_err = READ_ONCE(sk->sk_err);
+	if (err || sk_err) {
 		rec = NULL;
 
 		/* If err is already set on socket, return the same code */
-		if (sk->sk_err) {
-			ctx->async_wait.err = -sk->sk_err;
+		if (sk_err) {
+			ctx->async_wait.err = -sk_err;
 		} else {
 			ctx->async_wait.err = err;
 			tls_err_abort(sk, err);
-- 
2.50.1

[PATCH net-next v2 2/2] tls: make the record number wrap abort final

[Rishikesh Jethwani]

When a record number wraps, TLS aborts the connection with EBADMSG, but
only through sk_err, which is consumed when it is reported, by SO_ERROR
or sock_error(). After that the socket carries on under the same key
with record number 0:

 - RX: tls_rx_rec_wait() only checks sk_err while no record is ready.
   After the record with the last number is decrypted, tls_rx_rec_done()
   lets the parser pick up the next record. If it is already queued,
   recvmsg() keeps going because a record is ready, decrypts it with
   record number 0 and returns both, without an error. sk_err is only
   reported once the parser runs dry, and once it is cleared, receiving
   continues.

 - TX: the record using the last number is encrypted and queued, then
   fails to go out because sk_err is set. Once SO_ERROR is read, the
   next send transmits it and continues with record number 0.

Add tls_err_abort_final(), which also records the error in the TLS
context, where nothing clears it, and use it for the record number wrap
in both directions:

 - tls_rx_rec_wait() checks it before looking for a ready record. Data
   that was already decrypted can still be read, as before. poll()
   reports EPOLLERR, and EPOLLIN only for that data.

 - sendmsg(), the device push and tls_push_record() check it before
   taking or encrypting more data, and tls_push_sg() before handing
   anything to TCP, so that no queued record goes out after the abort.
   The device push doesn't count the refused record as sent, like the
   SW path, and tls_tx_records() doesn't raise sk_err again for it.

A KeyUpdate may be the last record received under a key; that already
pauses receive until a new key is installed. Don't abort the connection
in that case.

Add TLS 1.3 selftests. tx_wrap starts TX at the last record number, as
recseq_wrap does. The RX tests can't get their records that way, since
TX now refuses to send the record using the last number. In TLS 1.3 the
record number only enters the nonce, so TX instead starts at record
number 0 with the IV XORed with the second to last number, which
produces the last two RX records, then switches back to the plain IV
with a silent rekey to produce the wrapped record number 0.

Suggested-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 Documentation/networking/tls.rst  |  19 +++
 include/net/tls.h                 |   1 +
 net/tls/tls.h                     |  24 +++-
 net/tls/tls_device.c              |  12 +-
 net/tls/tls_main.c                |   8 +-
 net/tls/tls_sw.c                  |  44 ++++++-
 tools/testing/selftests/net/tls.c | 197 ++++++++++++++++++++++++++++++
 7 files changed, 293 insertions(+), 12 deletions(-)

diff --git a/Documentation/networking/tls.rst b/Documentation/networking/tls.rst
index 980c442d7161..ef1a3bacb72f 100644
--- a/Documentation/networking/tls.rst
+++ b/Documentation/networking/tls.rst
@@ -228,6 +228,25 @@ The number of successful and failed key updates is tracked in the
 ``TlsRxRekeyError`` statistics. The ``TlsRxRekeyReceived`` statistic
 counts KeyUpdate handshake messages that have been received.
 
+Record number exhaustion
+------------------------
+
+A record number must not wrap under the same key. When it would, the
+connection is aborted with ``EBADMSG``. On transmit, the record that
+uses the last 64-bit record number is not sent, and the send that
+completes it doesn't count its data as sent. On receive, that record is
+still delivered, but no record after it is. Either way, no further
+record data is sent, not even the rest of a partially sent record.
+Unlike other socket errors, this one is not cleared when it is reported,
+by ``SO_ERROR`` or otherwise: every later send, and every receive that
+needs another record, fails with ``EBADMSG``, and a new key does not
+change that. ``poll()`` reports ``EPOLLERR``, and ``EPOLLIN`` only while
+data that was already decrypted remains; that data can still be read.
+
+A TLS 1.3 KeyUpdate may be the last record received under a key. In
+that case receive is paused as for any other KeyUpdate, and continues
+once the new key is provided.
+
 Integrating in to userspace TLS library
 ---------------------------------------
 
diff --git a/include/net/tls.h b/include/net/tls.h
index e57bef58851e..c945f697fc91 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -251,6 +251,7 @@ struct tls_context {
 
 	bool splicing_pages;
 	bool pending_open_record_frags;
+	int final_err; /* set by tls_err_abort_final(), never cleared */
 
 	struct mutex tx_lock; /* protects partially_sent_* fields and
 			       * per-type TX fields
diff --git a/net/tls/tls.h b/net/tls/tls.h
index 60a37bdaaa25..517dcc452c67 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -142,6 +142,7 @@ void update_sk_prot(struct sock *sk, struct tls_context *ctx);
 
 int wait_on_pending_writer(struct sock *sk, long *timeo);
 void tls_err_abort(struct sock *sk, int err);
+void tls_err_abort_final(struct sock *sk, int err);
 void tls_strp_abort_strp(struct tls_strparser *strp, int err);
 
 int init_prot_info(struct tls_prot_info *prot,
@@ -307,17 +308,30 @@ static inline void tls_bigint_subtract(unsigned char *seq, int  n)
 	*p = cpu_to_be64(rcd_sn - n);
 }
 
-static inline void
-tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
-		      struct cipher_context *ctx)
+static inline bool
+__tls_advance_record_sn(struct tls_prot_info *prot, struct cipher_context *ctx)
 {
-	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
-		tls_err_abort(sk, -EBADMSG);
+	bool wrapped = tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);
 
 	if (prot->version != TLS_1_3_VERSION &&
 	    prot->cipher_type != TLS_CIPHER_CHACHA20_POLY1305)
 		tls_bigint_increment(ctx->iv + prot->salt_size,
 				     prot->iv_size);
+
+	return wrapped;
+}
+
+static inline void
+tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
+		      struct cipher_context *ctx)
+{
+	if (__tls_advance_record_sn(prot, ctx))
+		tls_err_abort_final(sk, -EBADMSG);
+}
+
+static inline int tls_final_err(const struct tls_context *ctx)
+{
+	return READ_ONCE(ctx->final_err);
 }
 
 static inline void
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 4cdfbb57ecff..5e95710e53fb 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -430,6 +430,7 @@ static int tls_push_data(struct sock *sk,
 	int tls_push_record_flags;
 	struct page_frag *pfrag;
 	size_t orig_size = size;
+	size_t rec_size = size;
 	u32 max_open_record_len;
 	bool more = false;
 	bool done = false;
@@ -444,6 +445,10 @@ static int tls_push_data(struct sock *sk,
 	if ((flags & (MSG_MORE | MSG_EOR)) == (MSG_MORE | MSG_EOR))
 		return -EINVAL;
 
+	rc = tls_final_err(tls_ctx);
+	if (unlikely(rc))
+		return rc;
+
 	if (unlikely(sk->sk_err))
 		return -sk->sk_err;
 
@@ -550,8 +555,13 @@ static int tls_push_data(struct sock *sk,
 					     ctx,
 					     record,
 					     tls_push_record_flags);
-			if (rc < 0)
+			if (rc < 0) {
+				/* the refused record isn't sent, don't count it */
+				if (tls_final_err(tls_ctx))
+					size = rec_size;
 				break;
+			}
+			rec_size = size;
 		}
 	} while (!done);
 
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index b8d5a843f761..d5605df59334 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -190,7 +190,7 @@ int tls_push_sg(struct sock *sk,
 		bvec_set_page(&bvec, p, size, offset);
 		iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bvec, 1, size);
 
-		ret = tcp_sendmsg_locked(sk, &msg, size);
+		ret = tls_final_err(ctx) ?: tcp_sendmsg_locked(sk, &msg, size);
 
 		if (ret != size) {
 			if (ret > 0) {
@@ -414,8 +414,12 @@ static __poll_t tls_sk_poll(struct file *file, struct socket *sock,
 	tls_ctx = tls_get_ctx(sk);
 	ctx = tls_sw_ctx_rx(tls_ctx);
 
+	/* the final abort outlives sk_err, only decrypted data is readable */
+	if (unlikely(tls_final_err(tls_ctx)))
+		mask |= EPOLLERR;
+
 	if ((skb_queue_empty_lockless(&ctx->rx_list) &&
-	     !tls_strp_msg_ready(ctx)) ||
+	     (!tls_strp_msg_ready(ctx) || tls_final_err(tls_ctx))) ||
 	    READ_ONCE(ctx->key_update_pending))
 		mask &= ~(EPOLLIN | EPOLLRDNORM);
 
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index b4535139438a..37ba09055220 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -78,6 +78,18 @@ noinline void tls_err_abort(struct sock *sk, int err)
 	sk_error_report(sk);
 }
 
+/* Like tls_err_abort(), but the error can't be cleared: SO_ERROR and
+ * sock_error() consume sk_err, final_err keeps failing reads and sends.
+ */
+void tls_err_abort_final(struct sock *sk, int err)
+{
+	struct tls_context *ctx = tls_get_ctx(sk);
+
+	if (!tls_final_err(ctx))
+		WRITE_ONCE(ctx->final_err, err);
+	tls_err_abort(sk, err);
+}
+
 static int __skb_nsg(struct sk_buff *skb, int offset, int len,
                      unsigned int recursion_level)
 {
@@ -458,7 +470,9 @@ int tls_tx_records(struct sock *sk, int flags)
 	}
 
 tx_err:
-	if (rc < 0 && rc != -EAGAIN && rc != -EINTR && rc != -ERESTARTSYS)
+	/* a final abort already reported its error */
+	if (rc < 0 && rc != -EAGAIN && rc != -EINTR && rc != -ERESTARTSYS &&
+	    !tls_final_err(tls_ctx))
 		tls_err_abort(sk, rc);
 
 	return rc;
@@ -628,6 +642,11 @@ static int tls_push_record(struct sock *sk, int flags,
 	int rc;
 	u32 i;
 
+	/* tls_push_sg() would refuse it, don't encrypt it */
+	rc = tls_final_err(tls_ctx);
+	if (unlikely(rc))
+		return rc;
+
 	if (!rec)
 		return 0;
 
@@ -802,6 +821,13 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 	}
 
 	while (msg_data_left(msg)) {
+		int err = tls_final_err(tls_ctx);
+
+		if (unlikely(err)) {
+			ret = err;
+			goto send_end;
+		}
+
 		if (sk->sk_err) {
 			ret = -sk->sk_err;
 			goto send_end;
@@ -1106,6 +1132,11 @@ tls_rx_rec_wait(struct sock *sk, bool nonblock, bool released, bool has_copied)
 	if (unlikely(ctx->key_update_pending))
 		return -EKEYEXPIRED;
 
+	/* checked even if a record is ready, unlike sk_err below */
+	ret = tls_final_err(tls_ctx);
+	if (unlikely(ret))
+		return ret;
+
 	timeo = sock_rcvtimeo(sk, nonblock);
 
 	while (!tls_strp_msg_ready(ctx)) {
@@ -1582,9 +1613,14 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 	rxm = strp_msg(darg->skb);
 	rxm->offset += prot->prepend_size;
 	rxm->full_len -= prot->overhead_size;
-	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 
-	return tls_check_pending_rekey(sk, tls_ctx, darg->skb);
+	err = tls_check_pending_rekey(sk, tls_ctx, darg->skb);
+	/* A KeyUpdate in the last record retires the key as usual */
+	if (__tls_advance_record_sn(prot, &tls_ctx->rx) &&
+	    !tls_sw_ctx_rx(tls_ctx)->key_update_pending)
+		tls_err_abort_final(sk, -EBADMSG);
+
+	return err;
 }
 
 int decrypt_skb(struct sock *sk, struct scatterlist *sgout)
@@ -2182,7 +2218,7 @@ bool tls_sw_sock_is_readable(struct sock *sk)
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
 
-	return tls_strp_msg_ready(ctx) ||
+	return (tls_strp_msg_ready(ctx) && !tls_final_err(tls_ctx)) ||
 		!skb_queue_empty(&ctx->rx_list);
 }
 
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index c4203401c4bf..e7dd77b4f3ff 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -2421,6 +2421,203 @@ TEST_F(tls, rekey_poll_delay)
 	}
 }
 
+/* Record number exhaustion, TLS 1.3 only. The record number only enters
+ * the nonce there, so a TX key whose IV is XORed with N sends record
+ * number N at its own record number 0, and TX never has to wrap.
+ */
+FIXTURE(tls_recseq)
+{
+	int fd, cfd;
+	bool notls;
+};
+
+FIXTURE_SETUP(tls_recseq)
+{
+	ulp_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
+}
+
+FIXTURE_TEARDOWN(tls_recseq)
+{
+	close(self->fd);
+	close(self->cfd);
+}
+
+static void tls_recseq_keys(struct tls_crypto_info_keys *tls,
+			    uint64_t rec_seq, uint64_t iv_xor)
+{
+	int i;
+
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128, tls, 0);
+	for (i = 0; i < 8; i++) {
+		tls->aes128.rec_seq[i] = rec_seq >> (56 - 8 * i);
+		tls->aes128.iv[i] ^= iv_xor >> (56 - 8 * i);
+	}
+}
+
+/* RX starts at the second to last record number, TX sends it and the
+ * last one at its record numbers 0 and 1.
+ */
+static void tls_recseq_set_keys(struct __test_metadata *_metadata,
+				int fd, int cfd)
+{
+	struct tls_crypto_info_keys tls;
+
+	tls_recseq_keys(&tls, 0, UINT64_MAX - 1);
+	ASSERT_EQ(setsockopt(fd, SOL_TLS, TLS_TX, &tls, tls.len), 0);
+	tls_recseq_keys(&tls, UINT64_MAX - 1, 0);
+	ASSERT_EQ(setsockopt(cfd, SOL_TLS, TLS_RX, &tls, tls.len), 0);
+}
+
+/* Silent rekey to the same key, so TX sends the wrapped record number 0 */
+static void tls_recseq_tx_wrapped(struct __test_metadata *_metadata, int fd)
+{
+	struct tls_crypto_info_keys tls;
+
+	tls_recseq_keys(&tls, 0, 0);
+	ASSERT_EQ(setsockopt(fd, SOL_TLS, TLS_TX, &tls, tls.len), 0);
+}
+
+static int tls_so_error(struct __test_metadata *_metadata, int fd)
+{
+	socklen_t len = sizeof(int);
+	int err = -1;
+
+	EXPECT_EQ(getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len), 0);
+	return err;
+}
+
+/* All three records are queued, only the first two may be received */
+TEST_F(tls_recseq, rx_wrap)
+{
+	struct pollfd pfd = { .events = POLLIN };
+	char const *test_str = "test_read";
+	int send_len = strlen(test_str) + 1;
+	char buf[3 * 10];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	tls_recseq_set_keys(_metadata, self->fd, self->cfd);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	tls_recseq_tx_wrapped(_metadata, self->fd);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), 2 * send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+	EXPECT_EQ(memcmp(buf + send_len, test_str, send_len), 0);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	/* reading SO_ERROR clears sk_err, the abort stays */
+	EXPECT_EQ(tls_so_error(_metadata, self->cfd), EBADMSG);
+	EXPECT_EQ(tls_so_error(_metadata, self->cfd), 0);
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	/* the parsed third record isn't readable */
+	pfd.fd = self->cfd;
+	EXPECT_EQ(poll(&pfd, 1, 0), 1);
+	EXPECT_EQ(pfd.revents, POLLERR);
+}
+
+TEST_F(tls_recseq, rx_wrap_splice)
+{
+	char const *test_str = "test_read";
+	int send_len = strlen(test_str) + 1;
+	char buf[10];
+	int p[2];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	ASSERT_GE(pipe(p), 0);
+
+	tls_recseq_set_keys(_metadata, self->fd, self->cfd);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	tls_recseq_tx_wrapped(_metadata, self->fd);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, send_len, 0), send_len);
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, send_len, 0), send_len);
+	EXPECT_EQ(read(p[0], buf, send_len), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+	EXPECT_EQ(read(p[0], buf, send_len), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+
+	EXPECT_EQ(tls_so_error(_metadata, self->cfd), EBADMSG);
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, send_len,
+			 SPLICE_F_NONBLOCK), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	close(p[0]);
+	close(p[1]);
+}
+
+/* A KeyUpdate may use the last record number, then a new key takes over */
+TEST_F(tls_recseq, rx_keyupdate)
+{
+	struct tls_crypto_info_keys tls12_1;
+	char const *test_str = "test_read";
+	int send_len = strlen(test_str) + 1;
+	char buf[10];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls12_1, 1);
+
+	tls_recseq_set_keys(_metadata, self->fd, self->cfd);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	tls_send_keyupdate(_metadata, self->fd);
+	ASSERT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_1,
+			     tls12_1.len), 0);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+	tls_recv_keyupdate(_metadata, self->cfd, 0);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT), -1);
+	EXPECT_EQ(errno, EKEYEXPIRED);
+	EXPECT_EQ(tls_so_error(_metadata, self->cfd), 0);
+
+	ASSERT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_1,
+			     tls12_1.len), 0);
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+}
+
+/* The record using the last record number is never sent */
+TEST_F(tls_recseq, tx_wrap)
+{
+	struct tls_crypto_info_keys tls;
+	char const *test_str = "test_read";
+	int send_len = strlen(test_str) + 1;
+	char buf[10];
+
+	if (self->notls)
+		SKIP(return, "no TLS support");
+
+	tls_recseq_keys(&tls, UINT64_MAX, 0);
+	ASSERT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls, tls.len), 0);
+	ASSERT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls, tls.len), 0);
+
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	/* reading SO_ERROR clears sk_err, the abort stays */
+	EXPECT_EQ(tls_so_error(_metadata, self->fd), EBADMSG);
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), -1);
+	EXPECT_EQ(errno, EBADMSG);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT), -1);
+	EXPECT_EQ(errno, EAGAIN);
+}
+
 struct raw_rec {
 	unsigned int plain_len;
 	unsigned char plain_data[100];
-- 
2.50.1

Re: [PATCH net-next v2 2/2] tls: make the record number wrap abort final

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With async encryption on the SW TX path, the record that uses
  the last record number is counted as sent and sendmsg() can return…
- [Medium] The new tls.rst text says, for an abort in either direction,
  that the error is not cleared when it is reported and that 'poll()…
- [Low] The handling of a KeyUpdate in the last record number differs
  between directions.
- [Low] The commit fixes behaviour reachable in the baseline: after a
  record number wraps, once sk_err has been consumed by SO_ERROR or…
- [Low] Documentation/networking/tls.rst limits the wrap exemption to 'A
  TLS 1.3 KeyUpdate ... receive ... continues once the new key is…
- [Low] The doc says that after the final abort 'every later send ...
  fails with EBADMSG'.
- [Low] On the sync SW TX path, whether the refused wrapped record is
  counted as sent still depends on a lockless read of `sk->sk_err ==…

--

tls: make the record number wrap abort final

The EBADMSG abort on a record number wrap now persists. It is recorded
in a new tls_context->final_err field that is never cleared. After a
wrap, RX delivers no more records unless the last one carried a
KeyUpdate, TX refuses to encrypt or push further records, poll()
reports EPOLLERR, and TLS 1.3 selftests are added.

> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 60a37bdaaa250..517dcc452c67f 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h

[ ... ]

> @@ -307,17 +308,30 @@ static inline void tls_bigint_subtract(unsigned char *seq, int  n)
>  	*p = cpu_to_be64(rcd_sn - n);
>  }
>  
> -static inline void
> -tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
> -		      struct cipher_context *ctx)
> +static inline bool
> +__tls_advance_record_sn(struct tls_prot_info *prot, struct cipher_context *ctx)
>  {
> -	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
> -		tls_err_abort(sk, -EBADMSG);

[Severity: Low]
The commit message describes behaviour that already exists in released
kernels. Once SO_ERROR or sock_error() has consumed sk_err after a wrap,
both RX and TX continue under the same key with record number 0.

Should this have a Fixes: tag for the commit that added the
tls_err_abort() wrap handling removed here? That would tell backporters
which trees are affected. If the patch is meant for net-next only, could
the commit message say so?

> +	bool wrapped = tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);

[ ... ]

> +static inline void
> +tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
> +		      struct cipher_context *ctx)
> +{
> +	if (__tls_advance_record_sn(prot, ctx))
> +		tls_err_abort_final(sk, -EBADMSG);
> +}

[Severity: Low]
On RX, tls_rx_one_record() exempts a KeyUpdate that uses the last record
number. There is no matching exemption on TX. Is that asymmetry
intended?

tls_advance_record_sn() runs after the record has been encrypted. That
is tls_do_encryption() on the SW path and tls_push_record() in
tls_device.c. It calls tls_err_abort_final() whatever the content type
is, and tls_push_sg() then refuses the record.

So a KeyUpdate that userspace sends via cmsg at rec_seq 2^64-1, which
RFC 8446 permits, is never sent. final_err is never cleared, so
installing the new key with setsockopt(TLS_TX) can't recover the
connection either.

The new KeyUpdate paragraph in tls.rst only covers the receive side.
Could it also describe the TX behaviour, or explain why kTLS refuses to
send what it accepts from the peer?

[ ... ]

> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index b8d5a843f7617..d5605df59334d 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c

[ ... ]

> @@ -414,8 +414,12 @@ static __poll_t tls_sk_poll(struct file *file, struct socket *sock,
>  	tls_ctx = tls_get_ctx(sk);
>  	ctx = tls_sw_ctx_rx(tls_ctx);
>  
> +	/* the final abort outlives sk_err, only decrypted data is readable */
> +	if (unlikely(tls_final_err(tls_ctx)))
> +		mask |= EPOLLERR;

[Severity: Medium]
The new documentation says "poll() reports EPOLLERR" for an abort in
either direction, but this is the only place the persistent EPOLLERR is
set. Does that hold for TX-only kTLS sockets?

build_proto_ops() only installs tls_sk_poll for configurations with RX:

net/tls/tls_main.c:build_proto_ops() {
	ops[TLS_SW  ][TLS_BASE] = ops[TLS_BASE][TLS_BASE];
	ops[TLS_SW  ][TLS_BASE].splice_eof	= tls_sw_splice_eof;
    ...
	ops[TLS_BASE][TLS_SW  ].poll		= tls_sk_poll;
    ...
	ops[TLS_SW  ][TLS_SW  ].poll		= tls_sk_poll;
    ...
	ops[TLS_HW  ][TLS_BASE] = ops[TLS_BASE][TLS_BASE];
    ...
}

A socket with only TLS_TX configured, SW or HW, uses tcp_poll(). That
reports EPOLLERR only while sk_err is set, and a TX wrap is exactly the
case where these sockets matter.

Once SO_ERROR consumes sk_err, as the tx_wrap selftest does, poll() no
longer reports EPOLLERR, while send() still fails with EBADMSG.

Even when tls_sk_poll() is used, it can return before reaching the new
check:

	if (unlikely(state != TCP_ESTABLISHED || shutdown & RCV_SHUTDOWN))
		return mask;

Once sk_err is cleared, wouldn't the documented EPOLLERR also be missing
in those states?

[ ... ]

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index b4535139438ae..37ba09055220d 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -458,7 +470,9 @@ int tls_tx_records(struct sock *sk, int flags)
>  	}
>  
>  tx_err:
> -	if (rc < 0 && rc != -EAGAIN && rc != -EINTR && rc != -ERESTARTSYS)
> +	/* a final abort already reported its error */
> +	if (rc < 0 && rc != -EAGAIN && rc != -EINTR && rc != -ERESTARTSYS &&
> +	    !tls_final_err(tls_ctx))
>  		tls_err_abort(sk, rc);

[Severity: Low]
With this change, tls_tx_records() no longer sets sk_err again when
final_err is set. On the sync SW TX path, bpf_exec_tx_verdict() still
uses a lockless read of sk_err to decide whether to subtract the refused
wrapped record from copied:

net/tls/tls_sw.c:bpf_exec_tx_verdict() {
    ...
	err = tls_push_record(sk, flags, record_type);
	if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
		*copied -= sk_msg_free(sk, msg);
		tls_free_open_rec(sk);
		err = -sk->sk_err;
	}
    ...
}

Can a concurrent SO_ERROR reader defeat this check? For example:

CPU1 (sendmsg, holds lock_sock)
tls_do_encryption()
  tls_advance_record_sn()
    tls_err_abort_final()
      sk_err = EBADMSG; sk_error_report()

CPU2
getsockopt(SO_ERROR)
  sock_error()
    xchg(&sk->sk_err, 0)

CPU1
tls_tx_records()
  tls_push_sg() returns final_err (-EBADMSG)
  sk_err is not set again here
bpf_exec_tx_verdict()
  sk->sk_err == EBADMSG is false, copied is not reduced

sendmsg() would then return a copied count that includes the refused
record. If sk_err is cleared between the == EBADMSG test and the
err = -sk->sk_err assignment, err becomes 0 instead.

The device path uses tls_final_err() for the same decision. Would
checking tls_final_err(tls_ctx) here too, with a single snapshot of the
error, close the window?

[ ... ]

> @@ -628,6 +642,11 @@ static int tls_push_record(struct sock *sk, int flags,
>  	int rc;
>  	u32 i;
>  
> +	/* tls_push_sg() would refuse it, don't encrypt it */
> +	rc = tls_final_err(tls_ctx);
> +	if (unlikely(rc))
> +		return rc;
> +

[Severity: Medium]
With async encryption, is the refused record still counted as sent?

The commit message says "The device push doesn't count the refused
record as sent, like the SW path". tls.rst says "the send that completes
it doesn't count its data as sent".

tls_do_encryption() advances the record number, and so raises the final
abort, even when crypto_aead_encrypt() returns -EINPROGRESS:

net/tls/tls_sw.c:tls_do_encryption() {
    ...
	/* Unhook the record from context if encryption is not failure */
	ctx->open_rec = NULL;
	tls_advance_record_sn(sk, prot, &tls_ctx->tx);
	return rc;
}

tls_push_record() then returns -EINPROGRESS without reaching
tls_tx_records(). bpf_exec_tx_verdict() skips the un-count because of
its err != -EINPROGRESS condition. tls_sw_sendmsg_locked() increments
num_async and ends up at send_end:

send_end:
	...
		err = tls_encrypt_async_wait(ctx);
	...
	if (test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask)) {
		cancel_delayed_work(&ctx->tx_work.work);
		tls_tx_records(sk, msg->msg_flags);
	}

end:
	ret = sk_stream_error(sk, msg->msg_flags, ret);
	return copied > 0 ? copied : ret;

tls_encrypt_async_wait() returns 0 because the encryption succeeded.
The -EBADMSG that tls_tx_records() gets from tls_push_sg() is ignored.

So sendmsg() reports the record's bytes as sent. Later flushes from
tls_encrypt_done() and tx_work_handler() are refused, so the data is
never sent.

The same seems to happen when the wrapped record is encrypted
synchronously while an earlier record is still pending async.
tls_tx_records() stops at the first record that isn't tx_ready and
returns 0, so bpf_exec_tx_verdict() leaves the bytes in copied.

The selftests only use synchronous software GCM, so they don't cover
this.

> @@ -802,6 +821,13 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
>  	}
>  
>  	while (msg_data_left(msg)) {
> +		int err = tls_final_err(tls_ctx);
> +
> +		if (unlikely(err)) {
> +			ret = err;
> +			goto send_end;
> +		}

[Severity: Low]
final_err is only checked inside the while (msg_data_left(msg)) loop.
Does a zero-length send() still succeed after the abort?

With size 0 and no cmsg, the loop never runs and num_async is 0.
sk_stream_error(sk, flags, 0) returns 0, so send() returns 0.

tls_push_data() on the device path checks tls_final_err() on entry, so
the same call fails there with EBADMSG. tls.rst says "every later send
... fails with EBADMSG".

[ ... ]

> @@ -1582,9 +1613,14 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
>  	rxm = strp_msg(darg->skb);
>  	rxm->offset += prot->prepend_size;
>  	rxm->full_len -= prot->overhead_size;
> -	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
>  
> -	return tls_check_pending_rekey(sk, tls_ctx, darg->skb);
> +	err = tls_check_pending_rekey(sk, tls_ctx, darg->skb);
> +	/* A KeyUpdate in the last record retires the key as usual */
> +	if (__tls_advance_record_sn(prot, &tls_ctx->rx) &&
> +	    !tls_sw_ctx_rx(tls_ctx)->key_update_pending)
> +		tls_err_abort_final(sk, -EBADMSG);

[Severity: Low]
tls.rst limits this exemption to "A TLS 1.3 KeyUpdate", but this check
only looks at key_update_pending. tls_check_pending_rekey() sets that
flag for any decrypted handshake record that starts with
TLS_HANDSHAKE_KEYUPDATE, whatever the version:

	if (hs_type == TLS_HANDSHAKE_KEYUPDATE) {
		struct tls_sw_context_rx *rx_ctx = ctx->priv_ctx_rx;

		WRITE_ONCE(rx_ctx->key_update_pending, true);

On TLS 1.2, a peer can put such a record at the last record number. Then
neither final_err nor sk_err is set. recv() returns EKEYEXPIRED forever,
SO_ERROR is 0, and poll() shows no EPOLLERR.

do_tls_setsockopt_conf() rejects a TLS 1.2 RX rekey with -EBUSY, so the
new key can never be installed.

Should the exemption also require prot->version == TLS_1_3_VERSION, so
TLS 1.2 gets the documented EBADMSG abort?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106193210.1152577-1-rjethwani%40purestorage.com