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] [day] [month] [year] [list]

Message-ID: <20200518172640.1b07cf46@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 18 May 2020 17:26:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
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 Tue, 19 May 2020 02:55:16 +0300 Vadim Fedorenko wrote:
> 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.

Yeah, as a matter of fact checking if copied is negative is just
papering over the issue. Cleaning up the record so it can be
re-submitted again would be better.

> > 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.

Cool, thanks!