netdev - Re: FW: [PATCH net] net/tls: sendfile fails with ktls offload

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

Date:   Fri, 25 Sep 2020 19:32:23 +0530
From:   rohit maheshwari <rohitm@...lsio.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     vakul.garg@....com, secdev <secdev@...lsio.com>
Subject: Re: FW: [PATCH net] net/tls: sendfile fails with ktls offload


> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Friday, September 25, 2020 3:27 AM
> To: Rohit Maheshwari <rohitm@...lsio.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; vakul.garg@....com; secdev <secdev@...lsio.com>
> Subject: Re: [PATCH net] net/tls: sendfile fails with ktls offload
>
> On Thu, 24 Sep 2020 13:20:25 +0530 Rohit Maheshwari wrote:
>> At first when sendpage gets called, if there is more data, 'more' in
>> tls_push_data() gets set which later sets pending_open_record_frags,
>> but when there is no more data in file left, and last time
>> tls_push_data() gets called, pending_open_record_frags doesn't get
>> reset. And later when
>> 2 bytes of encrypted alert comes as sendmsg, it first checks for
>> pending_open_record_frags, and since this is set, it creates a record
>> with
>> 0 data bytes to encrypt, meaning record length is prepend_size +
>> tag_size only, which causes problem.
> Agreed, looks like the value in pending_open_record_frags may be stale.
>
>>   We should set/reset pending_open_record_frags based on more bit.
> I think you implementation happens to work because there is always left over data when more is set, but I don't think that has to be the case.
Yes, with small file size, more bit won't be set, and so the existing code
works there. If more is not set, which means this should be the overall
record and so, we can continue putting header and TAG to make it a
complete record.
>
> Also shouldn't we update this field or destroy the record before the break on line 478?
If more is set, and payload is lesser than the max size, then we need to
hold on to get next sendpage and continue adding frags in the same record.
So I don't think we need to do any update or destroy the record. Please
correct me if I am wrong here.
>> Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
>> Signed-off-by: Rohit Maheshwari <rohitm@...lsio.com>
>> ---
>>   net/tls/tls_device.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index
>> b74e2741f74f..a02aadefd86e 100644
>> --- a/net/tls/tls_device.c
>> +++ b/net/tls/tls_device.c
>> @@ -492,11 +492,11 @@ static int tls_push_data(struct sock *sk,
>>   		if (!size) {
>>   last_record:
>>   			tls_push_record_flags = flags;
>> -			if (more) {
>> -				tls_ctx->pending_open_record_frags =
>> -						!!record->num_frags;
>> +			/* set/clear pending_open_record_frags based on more */
>> +			tls_ctx->pending_open_record_frags = !!more;
>> +
>> +			if (more)
>>   				break;
>> -			}
>>   
>>   			done = true;
>>   		}