net/tls: Fix flipped sign in tls_err_abort() calls

Commit Message

Daniel Jordan Oct. 21, 2021, 6:30 p.m. UTC

sk->sk_err appears to expect a positive value, a convention that ktls
doesn't always follow and that leads to memory corruption in other code.
For instance,

    [task1]
    tls_encrypt_done(..., err=<negative error from crypto request>)
      tls_err_abort(.., err)
        sk->sk_err = err;

    [task2]
    splice_from_pipe_feed
      ...
        tls_sw_do_sendpage
          if (sk->sk_err) {
            ret = -sk->sk_err;  // ret is positive

    splice_from_pipe_feed (continued)
      ret = actor(...)  // ret is still positive and interpreted as bytes
                        // written, resulting in underflow of buf->len and
                        // sd->len, leading to huge buf->offset and bogus
                        // addresses computed in later calls to actor()

Fix all tls_err_abort() callers to pass a negative error code
consistently and centralize the error-prone sign flip there, throwing in
a warning to catch future misuse.

Cc: [e-mail]
Fixes: c46234ebb4d1e ("tls: RX path for ktls")
Reported-by: [e-mail]
Signed-off-by: Daniel Jordan <[e-mail]>
---

I could be wrong about sk->sk_err expecting a positive value, but at
least the sign of the error code is inconsistent.  Open to suggestions.

 include/net/tls.h | 7 +++++--
 net/tls/tls_sw.c  | 8 ++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

Comments

Jakub Kicinski Oct. 21, 2021, 10:16 p.m. UTC | #1

On Thu, 21 Oct 2021 14:30:43 -0400 Daniel Jordan wrote:
> sk->sk_err appears to expect a positive value, a convention that ktls
> doesn't always follow and that leads to memory corruption in other code.
> For instance,
> 
>     [task1]
>     tls_encrypt_done(..., err=<negative error from crypto request>)
>       tls_err_abort(.., err)
>         sk->sk_err = err;
> 
>     [task2]
>     splice_from_pipe_feed
>       ...
>         tls_sw_do_sendpage
>           if (sk->sk_err) {
>             ret = -sk->sk_err;  // ret is positive
> 
>     splice_from_pipe_feed (continued)
>       ret = actor(...)  // ret is still positive and interpreted as bytes
>                         // written, resulting in underflow of buf->len and
>                         // sd->len, leading to huge buf->offset and bogus
>                         // addresses computed in later calls to actor()
> 
> Fix all tls_err_abort() callers to pass a negative error code
> consistently and centralize the error-prone sign flip there, throwing in
> a warning to catch future misuse.
> 
> Cc: [e-mail]
> Fixes: c46234ebb4d1e ("tls: RX path for ktls")
> Reported-by: [e-mail]
> Signed-off-by: Daniel Jordan <[e-mail]>
> ---
> 
> I could be wrong about sk->sk_err expecting a positive value, but at
> least the sign of the error code is inconsistent.  Open to suggestions.

Looks good to me, the WARN_ON_ONCE() may be a little heavy and fire
multiple times, but hopefully compiler will do a good enough job on
removing it from places where the argument can't be positive.

We should probably also fix this assignment:

			ctx->async_wait.err = sk->sk_err;

I think async_wait.err is expected to have a negative errno.

But that can be a separate patch.

Daniel Jordan Oct. 22, 2021, 5:50 p.m. UTC | #2

On Thu, Oct 21, 2021 at 03:16:03PM -0700, Jakub Kicinski wrote:
> Looks good to me

Thanks for looking.

> the WARN_ON_ONCE() may be a little heavy and fire
> multiple times, but hopefully compiler will do a good enough job on
> removing it from places where the argument can't be positive.

True, well we could uninline tls_err_abort() since it should always be a
slow path.  I'm kinda inclined to do that absent other opinions.

> We should probably also fix this assignment:
> 
> 			ctx->async_wait.err = sk->sk_err;
> 
> I think async_wait.err is expected to have a negative errno.
> 
> But that can be a separate patch.

Oh yeah, that does look wrong, I'll send it in the next version.

Patch

diff --git a/include/net/tls.h b/include/net/tls.h
index be4b3e1cac46..206505df2f1c 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -36,6 +36,7 @@ 
 
 #include <linux/types.h>
 #include <asm/byteorder.h>
+#include <linux/bug.h>
 #include <linux/crypto.h>
 #include <linux/socket.h>
 #include <linux/tcp.h>
@@ -468,7 +469,9 @@  static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
 
 static inline void tls_err_abort(struct sock *sk, int err)
 {
-	sk->sk_err = err;
+	WARN_ON_ONCE(err >= 0);
+	/* sk->sk_err should contain a positive error code. */
+	sk->sk_err = -err;
 	sk_error_report(sk);
 }
 
@@ -512,7 +515,7 @@  static inline void tls_advance_record_sn(struct sock *sk,
 					 struct cipher_context *ctx)
 {
 	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
-		tls_err_abort(sk, EBADMSG);
+		tls_err_abort(sk, -EBADMSG);
 
 	if (prot->version != TLS_1_3_VERSION &&
 	    prot->cipher_type != TLS_CIPHER_CHACHA20_POLY1305)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 4feb95e34b64..9705262749e6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -419,7 +419,7 @@  int tls_tx_records(struct sock *sk, int flags)
 
 tx_err:
 	if (rc < 0 && rc != -EAGAIN)
-		tls_err_abort(sk, EBADMSG);
+		tls_err_abort(sk, -EBADMSG);
 
 	return rc;
 }
@@ -763,7 +763,7 @@  static int tls_push_record(struct sock *sk, int flags,
 			       msg_pl->sg.size + prot->tail_size, i);
 	if (rc < 0) {
 		if (rc != -EINPROGRESS) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			if (split) {
 				tls_ctx->pending_open_record_frags = true;
 				tls_merge_open_record(sk, rec, tmp, orig_end);
@@ -1827,7 +1827,7 @@  int tls_sw_recvmsg(struct sock *sk,
 		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
 					 &chunk, &zc, async_capable);
 		if (err < 0 && err != -EINPROGRESS) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			goto recv_end;
 		}
 
@@ -2007,7 +2007,7 @@  ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 		}
 
 		if (err < 0) {
-			tls_err_abort(sk, EBADMSG);
+			tls_err_abort(sk, -EBADMSG);
 			goto splice_read_end;
 		}
 		ctx->decrypted = 1;