> -----Original Message----- > From: Boris Pismenny <[email protected]> > Sent: Wednesday, February 27, 2019 8:54 PM > To: Vakul Garg <[email protected]>; Dave Watson > <[email protected]> > Cc: Aviad Yehezkel <[email protected]>; [email protected]; > [email protected]; [email protected]; Eran Ben Elisha > <[email protected]> > 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 <[email protected]> > >> Sent: Wednesday, February 27, 2019 2:05 AM > >> To: Boris Pismenny <[email protected]> > >> Cc: [email protected]; [email protected]; > >> [email protected]; Vakul Garg <[email protected]>; > >> [email protected]; [email protected] > >> 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 <[email protected]> > >>> Reviewed-by: Eran Ben Elisha <[email protected]> > >>> --- > >>> 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.
