netdev - Re: [PATCH net v2] 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: <20201005164455.1f9c8c99@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 5 Oct 2020 16:44:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Rohit Maheshwari <rohitm@...lsio.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, secdev@...lsio.com
Subject: Re: [PATCH net v2] net/tls: sendfile fails with ktls offload

On Tue,  6 Oct 2020 00:13:13 +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.
>  We should set/reset pending_open_record_frags based on more bit.
> 
> Also incase if tls_do_allocation() fails, and if record len is only
> prepend_size, then destroy the record.
> 
> v1->v2:
> - handle tls_do_allocation() failure handling.
> 
> Signed-off-by: Rohit Maheshwari <rohitm@...lsio.com>
> ---
>  net/tls/tls_device.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index b74e2741f74f..f3efd53e31cf 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -463,17 +463,16 @@ static int tls_push_data(struct sock *sk,
>  			if (!record)
>  				break;
>  handle_error:
> -			if (record_type != TLS_RECORD_TYPE_DATA) {
> -				/* avoid sending partial
> -				 * record with type !=
> -				 * application_data
> -				 */
> -				size = orig_size;
> -				destroy_record(record);
> -				ctx->open_record = NULL;
> -			} else if (record->len > prot->prepend_size) {
> +			/* avoid sending partial record with type !=
> +			 * application_data
> +			 */
> +			if (record_type == TLS_RECORD_TYPE_DATA &&
> +			    record->len > prot->prepend_size)
>  				goto last_record;
> -			}
> +
> +			size = orig_size;
> +			destroy_record(record);
> +			ctx->open_record = NULL;

Yet, this still does not update pending_open_record_frags...

>  			break;
>  		}
> @@ -492,11 +491,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;
>  		}

Maybe I'm misunderstanding what you're fixing but I think you should
just do this:

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index b74e2741f74f..674964d5684b 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -418,7 +418,6 @@ static int tls_push_data(struct sock *sk,
        struct tls_context *tls_ctx = tls_get_ctx(sk);
        struct tls_prot_info *prot = &tls_ctx->prot_info;
        struct tls_offload_context_tx *ctx = tls_offload_ctx_tx(tls_ctx);
-       int more = flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE);
        struct tls_record_info *record = ctx->open_record;
        int tls_push_record_flags;
        struct page_frag *pfrag;
@@ -426,6 +425,7 @@ static int tls_push_data(struct sock *sk,
        u32 max_open_record_len;
        int copy, rc = 0;
        bool done = false;
+       bool more = false;
        long timeo;
 
        if (flags &
@@ -492,9 +492,8 @@ 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;
+                       if (flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE)) {
+                               more = true;
                                break;
                        }
 
@@ -526,6 +525,8 @@ static int tls_push_data(struct sock *sk,
                }
        } while (!done);
 
+       tls_ctx->pending_open_record_frags = more;
+
        if (orig_size - size > 0)
                rc = orig_size - size;