netdev - Re: Re: [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]

Message-ID: <439f7a6f-fdbd-8c6e-129d-c25f803e3e5e@chelsio.com>
Date:   Sun, 27 Sep 2020 00:13:31 +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: Re: [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
>> >
>
>> > 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.
>
> Agreed, if more is set we should continue appending.
>
> What I'm saying is that we may exit the loop on line 478 or 525 without
> updating pending_open_record_frags. So if pending_open_record_frags is
> set, we'd be in a position where there is no data in the record, yet
> pending_open_record_frags is set. Won't subsequent cmsg send not cause 
> a zero length record to be generated?
> Exit on line 478 can get triggered if sk_page_frag_refill() fails, and 
> then by
Exit on line 478 can get triggered if sk_page_frag_refill() fails,
and then by exiting, it will hit line 529 and will return 'rc =
orig_size - size', so I am sure we don't need to do anything else
there. Exit on line 525 will be, due to do_tcp_sendpage(), and I
think pending_open_record_frags won't be set if this is the last
record. And if it is not the last record, do_tcp_sendpage will be
failing for a complete and correct record, that doesn't need to be
destroyed and at this very moment pending_open_record_frags
will suggest that there is more data (unrelated to current failing
record), which actually is correct. I think, there won't be cmsg if
pending_open_record_frags is set.