Re: [PATCH] net/tls: Fix flipped sign in tls_err_abort() calls

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.