* [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls
@ 2021-10-21 18:30 Daniel Jordan
2021-10-21 22:16 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Jordan @ 2021-10-21 18:30 UTC (permalink / raw)
To: Boris Pismenny, John Fastabend, Daniel Borkmann, Jakub Kicinski,
David Miller, Dave Watson, Vakul Garg
Cc: netdev, linux-kernel, Daniel Jordan
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: stable@vger.kernel.org
Fixes: c46234ebb4d1e ("tls: RX path for ktls")
Reported-by: syzbot+b187b77c8474f9648fae@syzkaller.appspotmail.com
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
---
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(-)
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;
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls
2021-10-21 18:30 [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls Daniel Jordan
@ 2021-10-21 22:16 ` Jakub Kicinski
2021-10-22 17:50 ` Daniel Jordan
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2021-10-21 22:16 UTC (permalink / raw)
To: Daniel Jordan
Cc: Boris Pismenny, John Fastabend, Daniel Borkmann, David Miller,
Dave Watson, Vakul Garg, netdev
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: stable@vger.kernel.org
> Fixes: c46234ebb4d1e ("tls: RX path for ktls")
> Reported-by: syzbot+b187b77c8474f9648fae@syzkaller.appspotmail.com
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> ---
>
> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls
2021-10-21 22:16 ` Jakub Kicinski
@ 2021-10-22 17:50 ` Daniel Jordan
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Jordan @ 2021-10-22 17:50 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Boris Pismenny, John Fastabend, Daniel Borkmann, David Miller,
Dave Watson, Vakul Garg, netdev
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-22 17:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 18:30 [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls Daniel Jordan
2021-10-21 22:16 ` Jakub Kicinski
2021-10-22 17:50 ` Daniel Jordan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.