Message ID | 20211021183043.837139-1-daniel.m.jordan@oracle.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net/tls: Fix flipped sign in tls_err_abort() calls | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 38 this patch: 38 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Fixes tag looks correct |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 38 this patch: 38 |
netdev/header_inline | success | No static functions without inline keyword in header files |
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.
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.
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;
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(-)