netdev - Re: [PATCH net 3/4] tls: Fix mixing between async capable and async

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

Message-ID: <ab4db562-c6fe-646f-16fe-8fd990323d95@mellanox.com>
Date:   Tue, 26 Feb 2019 13:43:34 +0000
From:   Boris Pismenny <borisp@...lanox.com>
To:     Vakul Garg <vakul.garg@....com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "davejwatson@...com" <davejwatson@...com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [PATCH net 3/4] tls: Fix mixing between async capable and async


On 2/26/2019 2:38 PM, Vakul Garg wrote:
> 
> 
>> -----Original Message-----
>> From: Boris Pismenny <borisp@...lanox.com>
>> Sent: Tuesday, February 26, 2019 5:43 PM
>> To: aviadye@...lanox.com; davejwatson@...com;
>> john.fastabend@...il.com; daniel@...earbox.net; Vakul Garg
>> <vakul.garg@....com>; netdev@...r.kernel.org
>> Cc: eranbe@...lanox.com; borisp@...lanox.com
>> Subject: [PATCH net 3/4] tls: Fix mixing between async capable and async
>>
>> From: Eran Ben Elisha <eranbe@...lanox.com>
>>
>> Today, tls_sw_recvmsg is capable of using asynchronous mode to handle
>> application data TLS records. Moreover, it assumes that if the cipher can be
>> handled asynchronously, then all packets will be processed asynchronously.
>>
>> However, this assumption is not always true.
> 
> Could you please elaborate, what happens?
> 

When decryption doesn't occur asynchronously e.g. return code is not 
EINPROGRESS, then async should be turned off.

>> Specifically, for AES-GCM in
>> TLS1.2, it causes data corruption, and breaks user applications.
>>
>> This patch fixes this problem by separating the async capability from the
>> decryption operation result.
>>
>> Fixes: c0ab4732d4c6 ("net/tls: Do not use async crypto for non-data
>> records")
>> Signed-off-by: Eran Ben Elisha <eranbe@...lanox.com>
>> Reviewed-by: Boris Pismenny <borisp@...lanox.com>
>> ---
>>   net/tls/tls_sw.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
>> 4afa67b00aaf..f515cd7e984e 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -1693,7 +1693,8 @@ int tls_sw_recvmsg(struct sock *sk,
>>   		bool zc = false;
>>   		int to_decrypt;
>>   		int chunk = 0;
>> -		bool async;
>> +		bool async_capable;
>> +		bool async = false;
>>
>>   		skb = tls_wait_data(sk, psock, flags, timeo, &err);
>>   		if (!skb) {
>> @@ -1727,21 +1728,23 @@ int tls_sw_recvmsg(struct sock *sk,
>>
>>   		/* Do not use async mode if record is non-data */
>>   		if (ctx->control == TLS_RECORD_TYPE_DATA)
>> -			async = ctx->async_capable;
>> +			async_capable = ctx->async_capable;
>>   		else
>> -			async = false;
>> +			async_capable = false;
>>
>>   		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
>> -					 &chunk, &zc, async);
>> +					 &chunk, &zc, async_capable);
>>   		if (err < 0 && err != -EINPROGRESS) {
>>   			tls_err_abort(sk, EBADMSG);
>>   			goto recv_end;
>>   		}
>>
>> -		if (err == -EINPROGRESS)
>> +		if (err == -EINPROGRESS && async_capable) {
> Why do we need to check 'async_capable'?
> Do we get err == -EINPROGRESS even when async_capable is false?
>

I've missed this. I'll remove this and send V2.

>> +			async = true;
>>   			num_async++;
>> -		else if (prot->version == TLS_1_3_VERSION)
>> +		} else if (prot->version == TLS_1_3_VERSION) {
>>   			tlm->control = ctx->control;
>> +		}
>>
>>   		/* If the type of records being processed is not known yet,
>>   		 * set it to record type just dequeued. If it is already known,
>> --
>> 2.12.2
> 
>   
> 
>