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

[Jakub Kicinski <kuba@kernel.org>]

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.