Re: [PATCH 6.18] crypto: sa2ul - Fix HMAC key preparation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] crypto: sa2ul: negative index out-of-bounds read in
  sa_aead_setkey() [1]
--

commit 44d71286f88be3b8e50cd6afb3ed5b5d01ae3f54
Author: Eric Biggers <ebiggers@kernel.org>

crypto: sa2ul - Fix HMAC key preparation

This commit replaces the manual shash-based HMAC ipad/opad derivation in
sa_prepare_iopads() with the crypto library helpers hmac_sha1_preparekey()
and hmac_sha256_preparekey(), and removes the unused shash allocation and
indirect call.

> diff --git a/drivers/crypto/sa2ul.c b/drivers/crypto/sa2ul.c
> index 4a766e4351970..563e0f0d574ad 100644
> --- a/drivers/crypto/sa2ul.c
> +++ b/drivers/crypto/sa2ul.c

[ ... ]

> @@ -1785,7 +1720,6 @@ static int sa_aead_setkey(struct crypto_aead *authenc,
>  	if (key_idx >= 3)
>  		return -EINVAL;
>  
> -	ad->ctx = ctx;
>  	ad->enc_eng.eng_id = SA_ENG_ID_EM1;
>  	ad->enc_eng.sc_size = SA_CTX_ENC_TYPE1_SZ;
>  	ad->auth_eng.eng_id = SA_ENG_ID_AM1;

[Severity: Critical]
This problem wasn't introduced by this patch, but can a negative key_idx
cause an out-of-bounds read on mci_cbc_enc_no_iv_array[] and
mci_cbc_dec_no_iv_array[] [1]?

Unlike skcipher, the AEAD framework does not enforce a minimum encryption
key size before calling ->setkey().  When crypto_authenc_extractkeys()
extracts an encryption key with keys.enckeylen < 16, key_idx =
(keys.enckeylen >> 3) - 2 is negative, bypassing the signed key_idx >= 3
check:

drivers/crypto/sa2ul.c:sa_aead_setkey() {
    ...
	key_idx = (keys.enckeylen >> 3) - 2;
	if (key_idx >= 3)
		return -EINVAL;
    ...
	ad->mci_enc = mci_cbc_enc_no_iv_array[key_idx];
	ad->mci_dec = mci_cbc_dec_no_iv_array[key_idx];
    ...
}

This loads out-of-bounds memory into the hardware mode control instruction
pointers mci_enc and mci_dec.  Note that non-multiples of 8 (such as
enckeylen == 17) also pass this check, so validating keys.enckeylen with
aes_check_keylen() before computing key_idx would catch both.

[1] https://sashiko.dev/#/bug/linux-3f578a84-38a7-46fe-bc7b-117d71d9d394

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107192358.47287-1-ebiggers@kernel.org?part=1