Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()

[Simon Horman <horms@kernel.org>]

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
>