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: <20200924145714.761f7c6e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 24 Sep 2020 14:57:14 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Rohit Maheshwari <rohitm@...lsio.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, vakul.garg@....com,
        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.

Also shouldn't we update this field or destroy the record before the
break on line 478?

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