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.
