Re: [PATCH] l2tp: fix refcount leak in l2tp_nl_cmd_tunnel_create()

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]

On  Fri, Jun 12, 2026 at 00:54:49 +0800, WenTao Liang wrote:
> When l2tp_tunnel_register() fails, l2tp_nl_cmd_tunnel_create()
> directly frees the tunnel object with kfree(). This is incorrect
> because the tunnel's refcount was incremented to 2: once by
> l2tp_tunnel_create() (initial refcount=1) and again by the
> caller's refcount_inc() for a temporary reference. The successful
> path releases the temporary reference with l2tp_tunnel_put(),
> leaving the IDR to hold the remaining reference, but the error
> path bypasses reference counting entirely. As a result, the
> refcount stays at 2 while the memory is freed, which leaks
> references and violates the object's lifecycle that expects
> l2tp_tunnel_free() (via kfree_rcu()) when the refcount drops
> to zero.

(Same as the observation for the l2tp_ppp.c patch around the
l2tp_tunnel_register call, apologies for cutting/pasting...)

l2tp_tunnel_register only adds the newly created tunnel instance to
the per-net IDR once all the validation and initialisation steps have
completed successfully: there's no way for it to add the tunnel to the
IDR and still return an error.

Hence the new tunnel instance isn't visible to the rest of the system
if l2tp_tunnel_register returns an error.

As such, there's no potential for UAF when directly freeing the tunnel
structure in the way the code currently does.

Possibly there's value to altering the error path since it'll free the
tunnel instance via. l2tp_tunnel_free which will perform tidyup that
calling kfree() directly doesn't.

If the latter is a motivation it'd make more sense to say so in the
commit comment IMO since I don't think the UAF argument holds in the
current codebase.

> 
> Fix this by replacing kfree() with two l2tp_tunnel_put() calls:
> the first releases the temporary reference, and the second
> releases the initial reference, triggering the proper RCU‑safe
> cleanup.
> 
> Cc: stable@vger.kernel.org
> Fixes: 6b9f34239b00 ("l2tp: fix races in tunnel creation")
> Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
> ---
>  net/l2tp/l2tp_netlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
> index 59457c0c14aa..655bed496ffe 100644
> --- a/net/l2tp/l2tp_netlink.c
> +++ b/net/l2tp/l2tp_netlink.c
> @@ -245,7 +245,8 @@ static int l2tp_nl_cmd_tunnel_create(struct sk_buff *skb, struct genl_info *info
>  	refcount_inc(&tunnel->ref_count);
>  	ret = l2tp_tunnel_register(tunnel, net, &cfg);
>  	if (ret < 0) {
> -		kfree(tunnel);
> +		l2tp_tunnel_put(tunnel);
> +		l2tp_tunnel_put(tunnel);
>  		goto out;
>  	}
>  	ret = l2tp_tunnel_notify(&l2tp_nl_family, info, tunnel,
> -- 
> 2.50.1 (Apple Git-155)
> 
> 

-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]