linux-kernel - Re: [PATCH] xfrm: fix memory leak in xfrm_add

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: <aQ8Wj0fIH9KSEKg7@krikkit>
Date: Sat, 8 Nov 2025 11:08:15 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
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: [PATCH] xfrm: fix memory leak in xfrm_add_acquire()

2025-11-08, 05:10:54 +0000, Zilin Guan wrote:
> xfrm_add_acquire() constructs an xfrm_policy by calling
> xfrm_policy_construct(), which allocates the policy structure via
> xfrm_policy_alloc() and initializes its security context.
> 
> However, xfrm_add_acquire() currently releases the policy with kfree(),
> which skips the proper cleanup and causes a memory leak.
> 
> Fix this by calling xfrm_policy_destroy() instead of kfree() to
> properly release the policy and its associated resources, consistent
> with the cleanup path in xfrm_policy_construct().
> 
> Fixes: 980ebd25794f ("[IPSEC]: Sync series - acquire insert")
> Signed-off-by: Zilin Guan <zilin@....edu.cn>
> ---
>  net/xfrm/xfrm_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 010c9e6638c0..23c9bb42bb2a 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -3035,7 +3035,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	}
>  
>  	xfrm_state_free(x);
> -	kfree(xp);
> +	xfrm_policy_destroy(xp);

I agree there's something missing here, but that's not the right way
to fix this. You're calling this function:

void xfrm_policy_destroy(struct xfrm_policy *policy)
{
	BUG_ON(!policy->walk.dead);
[...]


And xfrm_add_acquire is not setting walk.dead. Have you tested your
patch?

Even if we did set walk.dead before calling xfrm_policy_destroy, we
would still be missing the xfrm_dev_policy_delete call that is done in
xfrm_policy_kill for the normal policy cleanup path.

I think we want something more like what xfrm_add_policy does if
insertion fails. In xfrm_policy_construct (which you mention in the
commit message), we don't have to worry about xfrm_dev_policy_delete
because xfrm_dev_policy_add has either not been called at all, or has
failed and does not need extra cleanup.

-- 
Sabrina