linux-kernel - Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake

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: <aTlGsMozNBaLDpNW@horms.kernel.org>
Date: Wed, 10 Dec 2025 10:08:48 +0000
From: Simon Horman <horms@...nel.org>
To: Wang Liang <wangliang74@...wei.com>
Cc: Chuck Lever <cel@...nel.org>, chuck.lever@...cle.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, brauner@...nel.org,
	kernel-tls-handshake@...ts.linux.dev, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, yuehaibing@...wei.com,
	zhangchangzhong@...wei.com
Subject: Re: [PATCH net] net/handshake: Fix null-ptr-deref in
 handshake_complete()

On Wed, Dec 10, 2025 at 10:51:11AM +0800, Wang Liang wrote:

...

> > Hi,
> > 
> > Clang 21.1.7 W=1 builds are rather unhappy about this:
> > 
> >    net/handshake/netlink.c:110:3: error: cannot jump from this goto statement to its label
> >      110 |                 goto out_status;
> >          |                 ^
> >    net/handshake/netlink.c:114:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
> >      114 |         FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> >          |                    ^
> >    net/handshake/netlink.c:104:3: error: cannot jump from this goto statement to its label
> >      104 |                 goto out_status;
> >          |                 ^
> >    net/handshake/netlink.c:114:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
> >      114 |         FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> >          |                    ^
> >    net/handshake/netlink.c:100:3: error: cannot jump from this goto statement to its label
> >      100 |                 goto out_status;
> >          |                 ^
> >    net/handshake/netlink.c:114:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
> >      114 |         FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> >          |                    ^
> > 
> > My undersatnding of the problem is as follows:
> > 
> > FD_PREPARE uses __cleanup to call class_fd_prepare_destructor when
> > resources when fdf goes out of scope.
> > 
> > Prior to this patch this was when the if (req) block was existed.
> > Either via return or a goto due to an error.
> > 
> > Now it is when handshake_nl_accept_doit() itself is exited.
> > Again via a return or a goto due to error.
> > 
> > But, importantly, such a goto can now occur before fdf is initialised.
> > Boom!
> 
> 
> Thanks for your analysis, you are right!
> 
> How about adding a null check before calling handshake_complete()?

I assumed the problem lies around the initialisation of fdf
and class_fd_prepare_destructor being called regardless of that
having occurred. But maybe I miss your point.

In any case, I would advocate an approach that left FD_PREPARE in a scope
where it is always called before the scope is exited.

> 
> Like:
> 
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 1d33a4675a48..b989456fc4c5 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -126,7 +126,8 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct
> genl_info *info)
>         }
> 
>  out_complete:
> -       handshake_complete(req, -EIO, NULL);
> +       if (req)
> +               handshake_complete(req, -EIO, NULL);
>  out_status:
>         trace_handshake_cmd_accept_err(net, req, NULL, err);
>         return err;
> 
> ------
> Best regards
> Wang Liang
>