Re: [net v2] net/tls: fix corrupted data in recvmsg

From: Vadim Fedorenko <vfedorenko@novek.ru>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Boris Pismenny <borisp@nvidia.com>,
	Aviad Yehezkel <aviadye@nvidia.com>,
	netdev@vger.kernel.org
Subject: Re: [net v2] net/tls: fix corrupted data in recvmsg
Date: Tue, 17 Nov 2020 00:59:54 +0000	[thread overview]
Message-ID: <f88588ce-03c7-74e0-1c43-0213d9133abd@novek.ru> (raw)
In-Reply-To: <20201116165454.5b5dd864@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On 17.11.2020 00:54, Jakub Kicinski wrote:
> On Tue, 17 Nov 2020 00:45:11 +0000 Vadim Fedorenko wrote:
>> On 17.11.2020 00:26, Jakub Kicinski wrote:
>>> On Sun, 15 Nov 2020 07:16:00 +0300 Vadim Fedorenko wrote:
>>>> If tcp socket has more data than Encrypted Handshake Message then
>>>> tls_sw_recvmsg will try to decrypt next record instead of returning
>>>> full control message to userspace as mentioned in comment. The next
>>>> message - usually Application Data - gets corrupted because it uses
>>>> zero copy for decryption that's why the data is not stored in skb
>>>> for next iteration. Revert check to not decrypt next record if
>>>> current is not Application Data.
>>>>
>>>> Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records")
>>>> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
>>>> ---
>>>>    net/tls/tls_sw.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>>>> index 95ab5545..2fe9e2c 100644
>>>> --- a/net/tls/tls_sw.c
>>>> +++ b/net/tls/tls_sw.c
>>>> @@ -1913,7 +1913,7 @@ int tls_sw_recvmsg(struct sock *sk,
>>>>    			 * another message type
>>>>    			 */
>>>>    			msg->msg_flags |= MSG_EOR;
>>>> -			if (ctx->control != TLS_RECORD_TYPE_DATA)
>>>> +			if (control != TLS_RECORD_TYPE_DATA)
>>> Sorry I wasn't clear enough, should this be:
>>>
>>> 	if (ctx->control != control)
>>>
>>> ? Otherwise if we get a control record first and then data record
>>> the code will collapse them, which isn't correct, right?
>>>   
>>>>    				goto recv_end;
>>>>    		} else {
>>>>    			break;
>> I think you mean when ctx->control is control record and control is
>> data record.
> Yup.
>
>> In this case control message will be decrypted without
>> zero copy and will be stored in skb for the next recvmsg, but will
>> not be returned together with data message.
> Could you point me to a line which breaks the loop in that case?
>
Sure!

		if (!control)
			control = tlm->control;
		else if (control != tlm->control)
			goto recv_end;

In that case control != tlm->control
Variable control is set only once and never changes again.

>> This behavior is the same
>> as for TLSv1.3 when record type is known only after decrypting.
>> But if we want completely different flow for TLSv1.2 and TLSv1.3
>> then changing to check difference in message types makes sense.