Re: [PATCH v2 1/9] crypto: Provide a wrapper for zeroizing crypto_aes_ctx

On Tue, Aug 04, 2026 at 09:42:34AM +0200, Thomas Huth wrote:
> On 03/08/2026 21.05, Eric Biggers wrote:
> > On Mon, Aug 03, 2026 at 11:44:20AM +0200, Thomas Huth wrote:
> > > From: Thomas Huth <[email protected]>
> > > 
> > > Several crypto drivers need to zeroize their local crypto_aes_ctx
> > > structures after use to avoid leaking key material on the stack.
> > > Currently some call sites do this with their own memzero_explicit()
> > > call, which is error-prone since it is easy to miss a return path
> > > (what already happened in some drivers). Some other call sites miss
> > > to clear crypto_aes_ctx completely.
> > > 
> > > Provide an aes_clear_ctx() helper that can be used with __cleanup()
> > > to automatically zeroize the context when it goes out of scope.
> > > 
> > > Signed-off-by: Thomas Huth <[email protected]>
> > > ---
> > >   include/crypto/aes.h | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/include/crypto/aes.h b/include/crypto/aes.h
> > > index 3279cfa546085..5ca7b1ab50e8c 100644
> > > --- a/include/crypto/aes.h
> > > +++ b/include/crypto/aes.h
> > > @@ -159,6 +159,19 @@ static inline int aes_check_keylen(size_t keylen)
> > >   int aes_expandkey(struct crypto_aes_ctx *ctx, const u8 *in_key,
> > >   		  unsigned int key_len);
> > > +/**
> > > + * aes_clear_ctx - Zeroize a crypto_aes_ctx structure
> > > + * @ctx:	The location of the context that should be zeroized
> > > + *
> > > + * Explicitly fills the crypto_aes_ctx with zeroes. This should be done
> > > + * once the context is not required anymore to avoid that its contents
> > > + * are leaked on the stack or heap.
> > > + */
> > > +static inline void aes_clear_ctx(struct crypto_aes_ctx *ctx)
> > > +{
> > > +	memzero_explicit(ctx, sizeof(*ctx));
> > > +}
> > 
> > Acked-by: Eric Biggers <[email protected]>
> > 
> > I guess we should start using __cleanup with type-specific zeroization
> > functions like this more often.
> 
> Yes, and I already got some more patches for other structure in the works
> already, just wanted to get review feedback on this series here first before
> sending them out / continuing that work.

By the way, in the function names could you consider using the verb
"zeroize" instead of "clear"?  So "aes_zeroize_ctx()".  We already have
sha3_zeroize_ctx(), shake_zeroize_ctx(), and chacha_zeroize_state().
And try 'git grep -i zeroize'.  It is the usual word used to mean
zeroization for crypto purposes specifically.

> > One gotcha is that __cleanup and 'goto'
> > should not be mixed in the same function; see the comment at
> > include/linux/cleanup.h line 148.  Patch 8 of this series doesn't follow
> > that in safexcel_aead_setkey().
> Ah, thanks for the hint, I wasn't aware of that recommendation yet!
> 
> As for safexcel_aead_setkey(), I think the change should be fine since the
> __cleanup() is declared at the very top of the function and not somewhere in
> an inner scope, so there is no way that the "gotos" could skip the cleanup
> here.
> 
> To get rid of the "gotos" here, I'd need to introduce another cleanup
> function for crypto_authenc_keys first, so if you insist of not mixing the
> __cleanup(aes_clear_ctx) with the gotos here, I think I'd rather drop that
> hunk from the patch for now, and provide another patch series with a cleanup
> for crypto_authenc_keys later that then adds the __cleanup() to both, struct
> crypto_authenc_keys keys and struct crypto_aes_ctx aes here and removes the
> gotos at the same time.
> 
> So WDYT, drop the hunk here for now and send a v3 with that, or keep the
> current v2 of this patch with its (hopefully unproblematic) ugliness?

Dropping that hunk sounds good.  I recommend avoiding the mixed
__cleanup and gotos even when they are done correctly, as it's easy to
get wrong and it raises questions.

Zeroziation of struct crypto_authenc_keys should just be removed, as
it's just a helper struct for parsing the key buffer.  It contains only
pointers into the key buffer, not the keys themselves.

- Eric