Message-ID: <sashiko-outbox-203214@kernel.org>
From: sashiko-bot@kernel.org
Subject: Re: [PATCH 6.18] crypto: sa2ul - Fix HMAC key preparation
Reply-To: sashiko-reviews@lists.linux.dev
To: "Eric Biggers" <ebiggers@kernel.org>
Cc: linux-crypto@vger.kernel.org, "Herbert Xu" <herbert@gondor.apana.org.au>
In-Reply-To: <20261107192358.47287-1-ebiggers@kernel.org>
References: <20261107192358.47287-1-ebiggers@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Sat, 07 Nov 2026 19:34:55 +0000
Precedence: list

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 *authe=
nc,
>  	if (key_idx >=3D 3)
>  		return -EINVAL;
> =20
> -	ad->ctx =3D ctx;
>  	ad->enc_eng.eng_id =3D SA_ENG_ID_EM1;
>  	ad->enc_eng.sc_size =3D SA_CTX_ENC_TYPE1_SZ;
>  	ad->auth_eng.eng_id =3D 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 =3D
(keys.enckeylen >> 3) - 2 is negative, bypassing the signed key_idx >=3D 3
check:

drivers/crypto/sa2ul.c:sa_aead_setkey() {
    ...
	key_idx =3D (keys.enckeylen >> 3) - 2;
	if (key_idx >=3D 3)
		return -EINVAL;
    ...
	ad->mci_enc =3D mci_cbc_enc_no_iv_array[key_idx];
	ad->mci_dec =3D 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 =3D=3D 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

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261107192358.4728=
7-1-ebiggers@kernel.org?part=3D1