netdev - Re: [PATCH net-next] net/tls: Removed redundant variable from 'struct tls_sw_context

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

Message-ID: <3e1dfaea-8895-69fc-476f-b80124713652@mellanox.com>
Date:   Thu, 12 Jul 2018 06:41:00 -0400
From:   Boris Pismenny <borisp@...lanox.com>
To:     Vakul Garg <vakul.garg@....com>, davem@...emloft.net,
        davejwatson@...com, netdev@...r.kernel.org
Cc:     aviadye@...lanox.com
Subject: Re: [PATCH net-next] net/tls: Removed redundant variable from 'struct
 tls_sw_context_rx'

Hi Vakul,

On 7/12/2018 7:03 AM, Vakul Garg wrote:
> The variable 'decrypted' in 'struct tls_sw_context_rx' is redundant and
> is being set/unset without purpose. Simplified the code by removing it.
> 

AFAIU, this variable has an important use here. It keeps the state 
whether the current record has been decrypted between invocations of the 
recv/splice system calls. Otherwise, some records would be decrypted 
more than once if the entire record was not read.

> Signed-off-by: Vakul Garg <vakul.garg@....com>
> ---
>   include/net/tls.h |  1 -
>   net/tls/tls_sw.c  | 87 ++++++++++++++++++++++++-------------------------------
>   2 files changed, 38 insertions(+), 50 deletions(-)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 70c273777fe9..528d0c2d6cc2 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -113,7 +113,6 @@ struct tls_sw_context_rx {
>   				struct poll_table_struct *wait);
>   	struct sk_buff *recv_pkt;
>   	u8 control;
> -	bool decrypted;
>   
>   	char rx_aad_ciphertext[TLS_AAD_SPACE_SIZE];
>   	char rx_aad_plaintext[TLS_AAD_SPACE_SIZE];
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 0d670c8adf18..e5f2de2c3fd6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -81,8 +81,6 @@ static int tls_do_decryption(struct sock *sk,
>   	rxm->full_len -= tls_ctx->rx.overhead_size;
>   	tls_advance_record_sn(sk, &tls_ctx->rx);
>   
> -	ctx->decrypted = true;
> -
>   	ctx->saved_data_ready(sk);
>   
>   out:
> @@ -756,6 +754,9 @@ int tls_sw_recvmsg(struct sock *sk,
>   	bool cmsg = false;
>   	int target, err = 0;
>   	long timeo;
> +	int page_count;
> +	int to_copy;
> +
>   
>   	flags |= nonblock;
>   
> @@ -792,46 +793,38 @@ int tls_sw_recvmsg(struct sock *sk,
>   			goto recv_end;
>   		}
>   
> -		if (!ctx->decrypted) {
> -			int page_count;
> -			int to_copy;
> -
> -			page_count = iov_iter_npages(&msg->msg_iter,
> -						     MAX_SKB_FRAGS);
> -			to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
> -			if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
> -			    likely(!(flags & MSG_PEEK)))  {
> -				struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> -				int pages = 0;
> -
> -				zc = true;
> -				sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> -				sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
> -					   TLS_AAD_SPACE_SIZE);
> -
> -				err = zerocopy_from_iter(sk, &msg->msg_iter,
> -							 to_copy, &pages,
> -							 &chunk, &sgin[1],
> -							 MAX_SKB_FRAGS,	false);
> -				if (err < 0)
> -					goto fallback_to_reg_recv;
> -
> -				err = decrypt_skb(sk, skb, sgin);
> -				for (; pages > 0; pages--)
> -					put_page(sg_page(&sgin[pages]));
> -				if (err < 0) {
> -					tls_err_abort(sk, EBADMSG);
> -					goto recv_end;
> -				}
> -			} else {
> +		page_count = iov_iter_npages(&msg->msg_iter, MAX_SKB_FRAGS);
> +		to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
> +
> +		if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
> +		    likely(!(flags & MSG_PEEK)))  {
> +			struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> +			int pages = 0;
> +
> +			zc = true;
> +			sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> +			sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
> +				   TLS_AAD_SPACE_SIZE);
> +			err = zerocopy_from_iter(sk, &msg->msg_iter, to_copy,
> +						 &pages, &chunk, &sgin[1],
> +						 MAX_SKB_FRAGS, false);
> +			if (err < 0)
> +				goto fallback_to_reg_recv;
> +
> +			err = decrypt_skb(sk, skb, sgin);
> +			for (; pages > 0; pages--)
> +				put_page(sg_page(&sgin[pages]));
> +			if (err < 0) {
> +				tls_err_abort(sk, EBADMSG);
> +				goto recv_end;
> +			}
> +		} else {
>   fallback_to_reg_recv:
> -				err = decrypt_skb(sk, skb, NULL);
> -				if (err < 0) {
> -					tls_err_abort(sk, EBADMSG);
> -					goto recv_end;
> -				}
> +			err = decrypt_skb(sk, skb, NULL);
> +			if (err < 0) {
> +				tls_err_abort(sk, EBADMSG);
> +				goto recv_end;
>   			}
> -			ctx->decrypted = true;
>   		}
>   
>   		if (!zc) {
> @@ -895,15 +888,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>   		goto splice_read_end;
>   	}
>   
> -	if (!ctx->decrypted) {
> -		err = decrypt_skb(sk, skb, NULL);
> +	err = decrypt_skb(sk, skb, NULL);
>   
> -		if (err < 0) {
> -			tls_err_abort(sk, EBADMSG);
> -			goto splice_read_end;
> -		}
> -		ctx->decrypted = true;
> +	if (err < 0) {
> +		tls_err_abort(sk, EBADMSG);
> +		goto splice_read_end;
>   	}
> +
>   	rxm = strp_msg(skb);
>   
>   	chunk = min_t(unsigned int, rxm->full_len, len);
> @@ -998,8 +989,6 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
>   	struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
>   	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
>   
> -	ctx->decrypted = false;
> -
>   	ctx->recv_pkt = skb;
>   	strp_pause(strp);
>   
>