Re: [PATCH v3 09/10] crypto: safexcel - Rework cleanup of sensitive structs in safexcel_aead_setkey

[Antoine Tenart]

On Thu, Aug 06, 2026 at 10:18:12AM +0200, Thomas Huth wrote:
> On 06/08/2026 10.10, Antoine Tenart wrote:
> > On Wed, Aug 05, 2026 at 01:57:47PM +0200, Thomas Huth wrote:
> >> From: Thomas Huth <thuth@xxxxxxxxxx>
> >>
> >> The crypto_authenc_keys structure only contains pointers to keys,
> >> but not the key data itself. So explicitly clearing the structure
> >> at the end of safexcel_aead_setkey() is not really necessary.
> >>
> >> On the other hand, the crypto_aes_ctx might contain sensitive information,
> >> so this structure should be cleaned up at the end instead. Do this
> >> now via the new __cleanup(aes_zeroize_ctx) marker.
> > 
> > Looking at other crypto drivers it seems zeroing the key pointers was
> > explicitly added (sometimes later) and my impression is a good chunk of
> > the users are zeroing it. I don't know whether removing that is fine or
> > not, my limited understanding is that provides in-depth defense against
> > leaking were the key reside in memory. Would love to see an explicit
> > statement from someone with that knowledge.
> 
> It has been suggested by Eric here (unless I got him wrong):
> 
>   https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@xxxxxxxxxx/
> 
> ... I should have maybe added that link to this patch description ...

Thanks for the link! Feel free to add it or not in the next revision.

> > (On the other hand mixing gotos and __cleanup is not advised but is that
> > an issue here? Or if zeroing crypto_authenc_keys is actually important
> > can we use __cleanup too?).
> > 
> >> --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> >> +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> >> @@ -407,17 +407,17 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >>   	struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
> >>   	struct safexcel_crypto_priv *priv = ctx->base.priv;
> >>   	struct crypto_authenc_keys keys;
> >> -	struct crypto_aes_ctx aes;
> >> -	int err = -EINVAL, i;
> >> +	struct crypto_aes_ctx aes __cleanup(aes_zeroize_ctx);
> >> +	int err, i;
> >>   	const char *alg;
> > 
> >> @@ -430,25 +430,25 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >>   	case SAFEXCEL_DES:
> >>   		err = verify_aead_des_key(ctfm, keys.enckey, keys.enckeylen);
> >>   		if (unlikely(err))
> >> -			goto badkey;
> >> +			return err;
> >>   		break;
> >>   	case SAFEXCEL_3DES:
> >>   		err = verify_aead_des3_key(ctfm, keys.enckey, keys.enckeylen);
> >>   		if (unlikely(err))
> >> -			goto badkey;
> >> +			return err;
> >>   		break;
> >>   	case SAFEXCEL_AES:
> >>   		err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
> >>   		if (unlikely(err))
> >> -			goto badkey;
> >> +			return err;
> >>   		break;
> >>   	case SAFEXCEL_SM4:
> >>   		if (unlikely(keys.enckeylen != SM4_KEY_SIZE))
> >> -			goto badkey;
> >> +			return err;
> > 
> > 'err' is uninitialized here. You can use '-EINVAL' instead.
> Oops, good catch, thanks! I will fix it in the next version (assuming that 
> removing the memzero_explicit is ok and we'll keep this patch...)