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

Message-ID: <20200925165235.5dba5d7d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 25 Sep 2020 16:52:35 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     rohit maheshwari <rohitm@...lsio.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        vakul.garg@....com, secdev <secdev@...lsio.com>
Subject: Re: [PATCH net] net/tls: sendfile fails with ktls offload

On Fri, 25 Sep 2020 19:32:23 +0530 rohit maheshwari wrote:
> > -----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.

Okay.

> > 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?

> >> 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;
> >>   		}