netdev - Re: [PATCH] net/tls: Fix flipped sign in tls_err

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211021151603.215ab29e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 21 Oct 2021 15:16:03 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
Cc:     Boris Pismenny <borisp@...dia.com>,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        Dave Watson <davejwatson@...com>,
        Vakul Garg <vakul.garg@....com>, netdev@...r.kernel.org
Subject: 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@...r.kernel.org
> Fixes: c46234ebb4d1e ("tls: RX path for ktls")
> Reported-by: syzbot+b187b77c8474f9648fae@...kaller.appspotmail.com
> Signed-off-by: Daniel Jordan <daniel.m.jordan@...cle.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.