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

[Antoine Tenart]

On Fri, Aug 07, 2026 at 09:40:58AM +0200, Antoine Tenart wrote:
> On Fri, Aug 07, 2026 at 09:06:36AM +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, see Eric's recommendation
> > here:
> > 
> >  https://lore.kernel.org/linux-crypto/20260804185402.GD2904385@xxxxxxxxxx/
> > 
> > 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.
> > 
> > Since __cleanup() and gotos should not be mixed in the same function,
> > replace the gotos with early return statements, which is fine now that
> > we dropped the memzero_explicit(&keys, sizeof(keys)) at the end.
> > 
> > Suggested-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> > Signed-off-by: Thomas Huth <thuth@xxxxxxxxxx>
> 
> Reviewed-by: Antoine Tenart <atenart@xxxxxxxxxx>

Actually I think we could do better here:

The AES key is expanded in safexcel_aead_setkey using aes_expandkey but
the result (the aes variable) is discarded and never used. This seems a
waste of CPU cycles for doing a validity check on the input key, which
can be replaced by a simple aes_check_keylen.

The same applies to the EIP93 patch for both chunks.

> > ---
> >  .../crypto/inside-secure/safexcel_cipher.c    | 27 ++++++++-----------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
> > index a8349b684693e..c331d81ac8a2d 100644
> > --- 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;
> >  
> >  	if (unlikely(crypto_authenc_extractkeys(&keys, key, len)))
> > -		goto badkey;
> > +		return -EINVAL;
> >  
> >  	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD) {
> >  		/* Must have at least space for the nonce here */
> >  		if (unlikely(keys.enckeylen < CTR_RFC3686_NONCE_SIZE))
> > -			goto badkey;
> > +			return -EINVAL;
> >  		/* last 4 bytes of key are the nonce! */
> >  		ctx->nonce = *(u32 *)(keys.enckey + keys.enckeylen -
> >  				      CTR_RFC3686_NONCE_SIZE);
> > @@ -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 -EINVAL;
> >  		break;
> >  	default:
> >  		dev_err(priv->dev, "aead: unsupported cipher algorithm\n");
> > -		goto badkey;
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma) {
> > @@ -486,24 +486,19 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> >  		break;
> >  	default:
> >  		dev_err(priv->dev, "aead: unsupported hash algorithm\n");
> > -		goto badkey;
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (safexcel_hmac_setkey(&ctx->base, keys.authkey, keys.authkeylen,
> >  				 alg, ctx->state_sz))
> > -		goto badkey;
> > +		return -EINVAL;
> >  
> >  	/* Now copy the keys into the context */
> >  	for (i = 0; i < keys.enckeylen / sizeof(u32); i++)
> >  		ctx->key[i] = cpu_to_le32(((u32 *)keys.enckey)[i]);
> >  	ctx->key_len = keys.enckeylen;
> >  
> > -	memzero_explicit(&keys, sizeof(keys));
> >  	return 0;
> > -
> > -badkey:
> > -	memzero_explicit(&keys, sizeof(keys));
> > -	return err;
> >  }
> >  
> >  static int safexcel_context_control(struct safexcel_cipher_ctx *ctx,
> > -- 
> > 2.55.0
> > 
>