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

Mat Martineau
2021-11-17T21:28:50Z

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

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@chronox.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@linux.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