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
| ||
|
Date: Wed, 27 Feb 2019 16:28:48 +0000 From: Vakul Garg <vakul.garg@....com> To: Boris Pismenny <borisp@...lanox.com>, Dave Watson <davejwatson@...com> CC: Aviad Yehezkel <aviadye@...lanox.com>, "john.fastabend@...il.com" <john.fastabend@...il.com>, "daniel@...earbox.net" <daniel@...earbox.net>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Eran Ben Elisha <eranbe@...lanox.com> Subject: RE: [PATCH net 4/4] tls: Fix tls_device receive > -----Original Message----- > From: Boris Pismenny <borisp@...lanox.com> > Sent: Wednesday, February 27, 2019 8:54 PM > To: Vakul Garg <vakul.garg@....com>; Dave Watson > <davejwatson@...com> > Cc: Aviad Yehezkel <aviadye@...lanox.com>; john.fastabend@...il.com; > daniel@...earbox.net; netdev@...r.kernel.org; Eran Ben Elisha > <eranbe@...lanox.com> > Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive > > > > On 2/27/2019 5:08 AM, Vakul Garg wrote: > > > > > >> -----Original Message----- > >> From: Dave Watson <davejwatson@...com> > >> Sent: Wednesday, February 27, 2019 2:05 AM > >> To: Boris Pismenny <borisp@...lanox.com> > >> Cc: aviadye@...lanox.com; john.fastabend@...il.com; > >> daniel@...earbox.net; Vakul Garg <vakul.garg@....com>; > >> netdev@...r.kernel.org; eranbe@...lanox.com > >> Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive > >> > >> On 02/26/19 02:12 PM, Boris Pismenny wrote: > >>> Currently, the receive function fails to handle records already > >>> decrypted by the device due to the commit mentioned below. > >>> > >>> This commit advances the TLS record sequence number and prepares the > >>> context to handle the next record. > >>> > >>> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on > >>> recv") > >>> Signed-off-by: Boris Pismenny <borisp@...lanox.com> > >>> Reviewed-by: Eran Ben Elisha <eranbe@...lanox.com> > >>> --- > >>> net/tls/tls_sw.c | 15 +++++++-------- > >>> 1 file changed, 7 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > >>> f515cd7e984e..85da10182d8d 100644 > >>> --- a/net/tls/tls_sw.c > >>> +++ b/net/tls/tls_sw.c > >>> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock > >>> *sk, struct sk_buff *skb, > >>> > >>> return err; > >>> } > >>> - > >>> - rxm->full_len -= padding_length(ctx, tls_ctx, skb); > >>> - > >>> - rxm->offset += prot->prepend_size; > >>> - rxm->full_len -= prot->overhead_size; > >>> - tls_advance_record_sn(sk, &tls_ctx->rx, version); > >>> - ctx->decrypted = true; > >>> - ctx->saved_data_ready(sk); > >>> } else { > >>> *zc = false; > >>> } > >>> > >>> + rxm->full_len -= padding_length(ctx, tls_ctx, skb); > >>> + rxm->offset += prot->prepend_size; > >>> + rxm->full_len -= prot->overhead_size; > >>> + tls_advance_record_sn(sk, &tls_ctx->rx, version); > >>> + ctx->decrypted = true; > >>> + ctx->saved_data_ready(sk); > >>> + > >>> return err; > >>> } > >> > >> This breaks the tls.control_msg test: > >> > >> [ RUN ] tls.control_msg > >> tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) > >> (18446744073709551614) == 0 (0) > >> tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) > >> (18446744073709551614) == 0 (0) > >> tls.control_msg: Test failed at step #8 > >> > >> So either control message handling needs to only call > >> decrypt_skb_update once, or we need a new flag or something to handle > >> the device case > > > > I prefer to remove variable 'decrypted' in context. > > This is no longer required as we already have an rx_list in context for > storing decrypted records. > > So for any record which we decrypted but did not return to user space > > (e.g. for the case when user used recv() and it lead to decryption of > > non-data record), we should it in rx_list. > > > > IMO this is inappropriate here, because packets decrypted by tls_device are > ready to be received, and there is no reason to bounce them through the > rx_list. My point was about preventing tls_sw_recvmsg() from calling decrypt_skb_update() with an already decrypted record. The test case failed because an already decrypted record got dequeued and passed to decrypt_skb_update() from tls_sw_recvmsg(). For packets decrypted by device, a check using skb->decrypted should be enough. For now, I think your patch is ok. I can submit a simplification patch for removing 'decrypted' from tls context later.
Powered by blists - more mailing lists