netdev - Re: [PATCH] net/tls: fix encryption error checking

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 19 May 2020 02:55:16 +0300
From:   Vadim Fedorenko <vfedorenko@...ek.ru>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Boris Pismenny <borisp@...lanox.com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/tls: fix encryption error checking

On 19.05.2020 02:23, Jakub Kicinski wrote:
> On Tue, 19 May 2020 02:05:29 +0300 Vadim Fedorenko wrote:
>> On 19.05.2020 01:30, Jakub Kicinski wrote:
>>>> tls_push_record can return -EAGAIN because of tcp layer. In that
>>>> case open_rec is already in the tx_record list and should not be
>>>> freed.
>>>> Also the record size can be more than the size requested to write
>>>> in tls_sw_do_sendpage(). That leads to overflow of copied variable
>>>> and wrong return code.
>>>>
>>>> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
>>>> Signed-off-by: Vadim Fedorenko <vfedorenko@...ek.ru>
>>> Doesn't this return -EAGAIN back to user space? Meaning even tho we
>>> queued the user space will try to send it again?
>> Before patch it was sending negative value back to user space.
>> After patch it sends the amount of data encrypted in last call. It is checked
>> by:
>>    return (copied > 0) ? copied : ret;
>> and returns -EAGAIN only if data is not sent to open record.
> I see, you're fixing two different bugs in one patch. Could you please
> split the fixes into two? (BTW no need for parenthesis around the
> condition in the ternary operator.) I think you need more fixes tags,
> too. Commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> already added one instance of the problem, right?
Sure, will split it into two. Also the problem with overflow is possible in
tls_sw_sendmsg(). But I'm not sure about correctness of freeing whole open
record in bpf_exec_tx_verdict.
> What do you think about Pooja's patch to consume the EAGAIN earlier?
> There doesn't seem to be anything reasonable we can do with the error
> anyway, not sure there is a point checking for it..
Yes, it's a good idea to consume this error earlier. I think it's better to fix
tls_push_record() instead of dealing with it every possible caller.

So I suggest to accept Pooja's patch and will resend only ssize_t checking fix.