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: <20251109150202.3685193-1-zilin@seu.edu.cn>
Date: Sun,  9 Nov 2025 15:02:02 +0000
From: Zilin Guan <zilin@....edu.cn>
To: sd@...asysnail.net
Cc: davem@...emloft.net,
	edumazet@...gle.com,
	herbert@...dor.apana.org.au,
	horms@...nel.org,
	jianhao.xu@....edu.cn,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	steffen.klassert@...unet.com,
	zilin@....edu.cn
Subject: Re: [PATCH] xfrm: fix memory leak in xfrm_add_acquire()

On Sat, Nov 08, 2025 at 11:08:15AM +0100, Sabrina Dubroca wrote:
> 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?

My apologies, I see the mistake now. To answer your question, I found 
this issue through static analysis and failed to test the patch properly 
before submission.

> 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.

Thank you for pointing this out. I agree that the xfrm_dev_policy_delete() 
call is also necessary.

> 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

Thank you for the detailed review and suggestion. I will follow the 
error handling pattern in xfrm_add_policy() and prepare a v2 patch 
accordingly.

Best regards,
Zilin Guan