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 PHC | |
Open Source and information security mailing list archives
| ||
|
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!
Powered by blists - more mailing lists