netdev - Re: [PATCH net-next] net: tls: Pass union tls_crypto_context pointer to memzero

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: <bb960821-a67b-4d61-afeb-ead10ea2a4dc@intel.com>
Date: Fri, 5 Jul 2024 15:54:59 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Horman <horms@...nel.org>
CC: Boris Pismenny <borisp@...dia.com>, John Fastabend
	<john.fastabend@...il.com>, <netdev@...r.kernel.org>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
	<pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: tls: Pass union tls_crypto_context pointer
 to memzero_explicit

On 7/5/24 15:41, Simon Horman wrote:
> Pass union tls_crypto_context pointer, rather than struct
> tls_crypto_info pointer, to memzero_explicit().
> 
> The address of the pointer is the same before and after.
> But the new construct means that the size of the dereferenced pointer type
> matches the size being zeroed. Which aids static analysis.
> 
> As reported by Smatch:
> 
>    .../tls_main.c:842 do_tls_setsockopt_conf() error: memzero_explicit() 'crypto_info' too small (4 vs 56)
> 
> No functional change intended.
> Compile tested only.
> 
> Signed-off-by: Simon Horman <horms@...nel.org>

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
one small nitpick only

> ---
>   net/tls/tls_main.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 90b7f253d363..e712b2faeb81 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -616,6 +616,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   	struct tls_crypto_info *alt_crypto_info;
>   	struct tls_context *ctx = tls_get_ctx(sk);
>   	const struct tls_cipher_desc *cipher_desc;
> +	union tls_crypto_context *crypto_ctx;
>   	int rc = 0;
>   	int conf;
>   
> @@ -623,13 +624,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   		return -EINVAL;
>   
>   	if (tx) {
> -		crypto_info = &ctx->crypto_send.info;
> +		crypto_ctx = &ctx->crypto_send;
>   		alt_crypto_info = &ctx->crypto_recv.info;
>   	} else {
> -		crypto_info = &ctx->crypto_recv.info;
> +		crypto_ctx = &ctx->crypto_recv;
>   		alt_crypto_info = &ctx->crypto_send.info;
>   	}
>   
> +	crypto_info = &crypto_ctx->info;
> +
>   	/* Currently we don't support set crypto info more than one time */
>   	if (TLS_CRYPTO_INFO_READY(crypto_info))
>   		return -EBUSY;
> @@ -710,7 +713,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   	return 0;
>   
>   err_crypto_info:
> -	memzero_explicit(crypto_info, sizeof(union tls_crypto_context));
> +	memzero_explicit(crypto_ctx, sizeof(union tls_crypto_context));

nit: That's a good fix to aid static analyzers, and reviewers.
Now it's also easy to follow the standard style and pass
sizeof(*crypto_ctx) instead of the type.

>   	return rc;
>   }