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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aCM8Y9iNXmbuPD5G@Antony2201.local>
Date: Tue, 13 May 2025 14:34:43 +0200
From: Antony Antony <antony@...nome.org>
To: Zilin Guan <zilin@....edu.cn>
Cc: steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, jianhao.xu@....edu.cn
Subject: Re: [RFC PATCH] xfrm: use kfree_sensitive() for SA secret zeroization

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