linux-kernel - Re: [PATCH v3 3/4] security: DH - remove dead code for zero padding

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: <f98dbf21-50d1-7847-18cb-2f5093a8ac8@linux.intel.com>
Date:   Wed, 17 Nov 2021 13:28:46 -0800 (PST)
From:   Mat Martineau <mathew.j.martineau@...ux.intel.com>
To:     Stephan Müller <smueller@...onox.de>
cc:     herbert@...dor.apana.org.au, ebiggers@...nel.org,
        Jarkko Sakkinen <jarkko@...nel.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        keyrings <keyrings@...r.kernel.org>, simo@...hat.com
Subject: Re: [PATCH v3 3/4] security: DH - remove dead code for zero
 padding

On Mon, 15 Nov 2021, Stephan Müller wrote:

> Remove the specific code that adds a zero padding that was intended
> to be invoked when the DH operation result was smaller than the
> modulus. However, this cannot occur any more these days because the
> function mpi_write_to_sgl is used in the code path that calculates the
> shared secret in dh_compute_value. This MPI service function guarantees
> that leading zeros are introduced as needed to ensure the resulting data
> is exactly as long as the modulus. This implies that the specific code
> to add zero padding is dead code which can be safely removed.
>
> Signed-off-by: Stephan Mueller <smueller@...onox.de>
> ---
> security/keys/dh.c | 25 ++++---------------------
> 1 file changed, 4 insertions(+), 21 deletions(-)

Hi Stephan -

Thanks for the cleanup!

Acked-by: Mat Martineau <mathew.j.martineau@...ux.intel.com>



>
> diff --git a/security/keys/dh.c b/security/keys/dh.c
> index 1abfa70ed6e1..56e12dae4534 100644
> --- a/security/keys/dh.c
> +++ b/security/keys/dh.c
> @@ -141,7 +141,7 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc)
>  * 'dlen' must be a multiple of the digest size.
>  */
> static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
> -		   u8 *dst, unsigned int dlen, unsigned int zlen)
> +		   u8 *dst, unsigned int dlen)
> {
> 	struct shash_desc *desc = &sdesc->shash;
> 	unsigned int h = crypto_shash_digestsize(desc->tfm);
> @@ -158,22 +158,6 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
> 		if (err)
> 			goto err;
>
> -		if (zlen && h) {
> -			u8 tmpbuffer[32];
> -			size_t chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
> -			memset(tmpbuffer, 0, chunk);
> -
> -			do {
> -				err = crypto_shash_update(desc, tmpbuffer,
> -							  chunk);
> -				if (err)
> -					goto err;
> -
> -				zlen -= chunk;
> -				chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
> -			} while (zlen);
> -		}
> -
> 		if (src && slen) {
> 			err = crypto_shash_update(desc, src, slen);
> 			if (err)
> @@ -198,7 +182,7 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
>
> static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
> 				 char __user *buffer, size_t buflen,
> -				 uint8_t *kbuf, size_t kbuflen, size_t lzero)
> +				 uint8_t *kbuf, size_t kbuflen)
> {
> 	uint8_t *outbuf = NULL;
> 	int ret;
> @@ -211,7 +195,7 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
> 		goto err;
> 	}
>
> -	ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len, lzero);
> +	ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len);
> 	if (ret)
> 		goto err;
>
> @@ -384,8 +368,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
> 		}
>
> 		ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, outbuf,
> -					    req->dst_len + kdfcopy->otherinfolen,
> -					    outlen - req->dst_len);
> +					    req->dst_len + kdfcopy->otherinfolen);
> 	} else if (copy_to_user(buffer, outbuf, req->dst_len) == 0) {
> 		ret = req->dst_len;
> 	} else {
> -- 
> 2.33.1
>
>
>
>
>

--
Mat Martineau
Intel