Re: [PATCH v6 2/9] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing explicit scrub of local stack buffer `buf` in `ctr_aes_crypt()` (standard AES algorithm).
- [High] Missing explicit scrub of temp buffers in `gcm_sg_walk` structures and AES keys in `gcm_aes_crypt()`.
--

--- Patch [2]: [PATCH v6 2/9] s390/crypto: Fix missing scrub of temp buffers with PAES algorithm ---
commit 0f77835bffb943e6b87472b41b755be562bb23c4
Author: Harald Freudenberger <[email protected]>

s390/crypto: Fix missing scrub of temp buffers with PAES algorithm

This commit explicitly zeroizes temporary stack buffers used for partial
block processing in the paes algorithm. It also updates the scrubbing
logic in the xts parameter preparation function to clear the entire
parameter block rather than just the key field.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index ccc7da106c3a0..d075b0241f1f5 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c
> @@ -1044,6 +1044,7 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>  	}
>  
>  out:
> +	memzero_explicit(buf, sizeof(buf));

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing the missing
scrub of buf here, there appears to be an identical pre-existing issue in the
standard aes implementation.

In arch/s390/crypto/aes_s390.c:ctr_aes_crypt(), the local stack buffer buf
is populated with cryptographic data when processing a final partial block,
but it returns without scrubbing the buffer:

		ret = skcipher_walk_done(&walk, 0);
	}

	return ret;
}

Since this path is reachable by unprivileged users invoking the ctr(aes)
algorithm with unaligned lengths, does ctr_aes_crypt() need a similar
memzero_explicit() call before returning to avoid leaving sensitive data on
the kernel stack?

[Severity: High]
This is another pre-existing issue in the same subsystem, but looking at
gcm_aes_crypt() in arch/s390/crypto/aes_s390.c, there are error paths that
bypass the existing memzero_explicit() cleanup completely:

			return -ENOMEM;
		if (gcm_out_walk_done(&gw_out, n) != n)
			return -ENOMEM;

If these error paths are taken when processing fragmented input data, does this
leave the aes key and hash subkey on the kernel stack?

Additionally, the normal exit path cleans the param block but doesn't appear
to clean the buf fields in the gcm_sg_walk structures (gw_in and gw_out):

	memzero_explicit(&param, sizeof(param));
	return ret;
}

Should these temporary buffers and early return paths also be scrubbed?

>  	pr_debug("rc=%d\n", rc);
>  	return rc;
>  }
> @@ -1377,7 +1378,7 @@ static inline int __xts_2keys_prep_param(struct s390_pxts_ctx *ctx,
>  		memcpy(param->init, pcc_param.xts, 16);
>  	}
>  
> -	memzero_explicit(pcc_param.key, sizeof(pcc_param.key));
> +	memzero_explicit(&pcc_param, sizeof(pcc_param));
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2