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

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

[Zilin Guan]

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.

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) {
+		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

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

[Antony Antony]

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")