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

[Thomas Huth <thuth@redhat.com>]

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@redhat.com>
>>
>> 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@google.com/

... I should have maybe added that link to this patch description ...

> (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...)

  Thomas