Re: [RFC PATCH] xfrm: use kfree_sensitive() for SA secret zeroization

[Antony Antony <antony@phenome.org>]

On Mon, May 12, 2025 at 09:28:08AM +0000, Zilin Guan wrote:
> The XFRM subsystem supports redaction of Security Association (SA)
> secret material when CONFIG_SECURITY lockdown for XFRM secrets is active.
> High-level copy_to_user_* APIs already omit secret fields, but the
> state destruction path still invokes plain kfree(), which does not zero
> the underlying memory before freeing. This can leave SA keys and
> other confidential data in memory, risking exposure via post-free
> vulnerabilities.
> 
> This patch modifies __xfrm_state_destroy() so that, if SA secret
> redaction is enabled, it calls kfree_sensitive() on the aead, aalg and
> ealg structs, ensuring secure zeroization prior to deallocation. When
> redaction is disabled, the existing kfree() behavior is preserved.
> 
> Note that xfrm_redact() is the identical helper function as implemented
> in net/xfrm/xfrm_user.c. And this patch is an RFC to seek feedback on
> whether this change is appropriate and if there is a better patch method.

I would prefer to use the existing one than an additional copy. If it is 
necessary. See the comment bellow.

> 
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  net/xfrm/xfrm_state.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 341d79ecb5c2..b6f2c329ea9d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -593,15 +593,28 @@ void xfrm_state_free(struct xfrm_state *x)
>  }
>  EXPORT_SYMBOL(xfrm_state_free);
>  
> +static bool xfrm_redact(void)
> +{
> +	return IS_ENABLED(CONFIG_SECURITY) &&
> +		security_locked_down(LOCKDOWN_XFRM_SECRET);
> +}
> +
>  static void ___xfrm_state_destroy(struct xfrm_state *x)
>  {
> +	bool redact_secret = xfrm_redact();
>  	if (x->mode_cbs && x->mode_cbs->destroy_state)
>  		x->mode_cbs->destroy_state(x);
>  	hrtimer_cancel(&x->mtimer);
>  	timer_delete_sync(&x->rtimer);
> -	kfree(x->aead);
> -	kfree(x->aalg);
> -	kfree(x->ealg);
> +	if (redact_secret) {

I recommend using kfree_sensitive() unconditionally.
This code is not in the fast path, so the overhead compared to kfree() would 
be acceptable?

It's generally better to always wipe key material explicitly.
When I originally  submitted the redact patch [1], I assumed that in 
environments with a good LSM(like AppArmor or SELinux) enabled, 
kfree_sensitive() would be the default kfree().

If kfree_sensitive() is called unconditionally, the call to xfrm_redact() in 
this file won not be necessary.


> +		kfree_sensitive(x->aead);
> +		kfree_sensitive(x->aalg);
> +		kfree_sensitive(x->ealg);
> +	} else {
> +		kfree(x->aead);
> +		kfree(x->aalg);
> +		kfree(x->ealg);
> +	}
>  	kfree(x->calg);
>  	kfree(x->encap);
>  	kfree(x->coaddr);
> -- 
> 2.34.1

-antony

[1] Fixes: c7a5899eb26e ("xfrm: redact SA secret with lockdown confidentiality")