* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-09 11:58 [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete() Wang Liang
@ 2025-12-09 14:39 ` Chuck Lever
2025-12-10 2:50 ` Wang Liang
2025-12-09 18:06 ` Simon Horman
2025-12-10 1:36 ` kernel test robot
2 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2025-12-09 14:39 UTC (permalink / raw)
To: Wang Liang, Chuck Lever, davem, edumazet, kuba, pabeni,
Simon Horman, Christian Brauner
Cc: kernel-tls-handshake, netdev, linux-kernel, yuehaibing,
zhangchangzhong
On Tue, Dec 9, 2025, at 6:58 AM, Wang Liang wrote:
> A null pointer dereference in handshake_complete() was observed [1].
>
> When handshake_req_next() return NULL in handshake_nl_accept_doit(),
> function handshake_complete() will be called unexpectedly which triggers
> this crash. Fix it by goto out_status when req is NULL.
>
> [1]
> Oops: general protection fault, probably for non-canonical address
> 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
> RIP: 0010:handshake_complete+0x36/0x2b0 net/handshake/request.c:288
> Call Trace:
> <TASK>
> handshake_nl_accept_doit+0x32d/0x7e0 net/handshake/netlink.c:129
> genl_family_rcv_msg_doit+0x204/0x300 net/netlink/genetlink.c:1115
> genl_family_rcv_msg+0x436/0x670 net/netlink/genetlink.c:1195
> genl_rcv_msg+0xcc/0x170 net/netlink/genetlink.c:1210
> netlink_rcv_skb+0x14c/0x430 net/netlink/af_netlink.c:2550
> genl_rcv+0x2d/0x40 net/netlink/genetlink.c:1219
> netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
> netlink_unicast+0x878/0xb20 net/netlink/af_netlink.c:1344
> netlink_sendmsg+0x897/0xd70 net/netlink/af_netlink.c:1894
> sock_sendmsg_nosec net/socket.c:727 [inline]
> __sock_sendmsg net/socket.c:742 [inline]
> ____sys_sendmsg+0xa39/0xbf0 net/socket.c:2592
> ___sys_sendmsg+0x121/0x1c0 net/socket.c:2646
> __sys_sendmsg+0x155/0x200 net/socket.c:2678
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x5f/0x350 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> </TASK>
>
> Fixes: fe67b063f687 ("net/handshake: convert handshake_nl_accept_doit()
> to FD_PREPARE()")
> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
> net/handshake/netlink.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 1d33a4675a48..cdaea8b8d004 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -106,25 +106,26 @@ int handshake_nl_accept_doit(struct sk_buff *skb,
> struct genl_info *info)
>
> err = -EAGAIN;
> req = handshake_req_next(hn, class);
> - if (req) {
> - sock = req->hr_sk->sk_socket;
> -
> - FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> - if (fdf.err) {
> - err = fdf.err;
> - goto out_complete;
> - }
> -
> - get_file(sock->file); /* FD_PREPARE() consumes a reference. */
> - err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
> - if (err)
> - goto out_complete; /* Automatic cleanup handles fput */
> -
> - trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
> - fd_publish(fdf);
> - return 0;
> + if (!req)
> + goto out_status;
> +
> + sock = req->hr_sk->sk_socket;
> +
> + FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> + if (fdf.err) {
> + err = fdf.err;
> + goto out_complete;
> }
>
> + get_file(sock->file); /* FD_PREPARE() consumes a reference. */
> + err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
> + if (err)
> + goto out_complete; /* Automatic cleanup handles fput */
> +
> + trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
> + fd_publish(fdf);
> + return 0;
> +
> out_complete:
> handshake_complete(req, -EIO, NULL);
> out_status:
> --
> 2.34.1
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/kernel-tls-handshake/aScekpuOYHRM9uOd@morisot.1015granger.net/T/#m7cfa5c11efc626d77622b2981591197a2acdd65e
--
Chuck Lever
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-09 14:39 ` Chuck Lever
@ 2025-12-10 2:50 ` Wang Liang
0 siblings, 0 replies; 7+ messages in thread
From: Wang Liang @ 2025-12-10 2:50 UTC (permalink / raw)
To: Chuck Lever, Chuck Lever, davem, edumazet, kuba, pabeni,
Simon Horman, Christian Brauner
Cc: kernel-tls-handshake, netdev, linux-kernel, yuehaibing,
zhangchangzhong
在 2025/12/9 22:39, Chuck Lever 写道:
>
> On Tue, Dec 9, 2025, at 6:58 AM, Wang Liang wrote:
>> A null pointer dereference in handshake_complete() was observed [1].
>>
>> When handshake_req_next() return NULL in handshake_nl_accept_doit(),
>> function handshake_complete() will be called unexpectedly which triggers
>> this crash. Fix it by goto out_status when req is NULL.
>>
>> [1]
>> Oops: general protection fault, probably for non-canonical address
>> 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
>> RIP: 0010:handshake_complete+0x36/0x2b0 net/handshake/request.c:288
>> Call Trace:
>> <TASK>
>> handshake_nl_accept_doit+0x32d/0x7e0 net/handshake/netlink.c:129
>> genl_family_rcv_msg_doit+0x204/0x300 net/netlink/genetlink.c:1115
>> genl_family_rcv_msg+0x436/0x670 net/netlink/genetlink.c:1195
>> genl_rcv_msg+0xcc/0x170 net/netlink/genetlink.c:1210
>> netlink_rcv_skb+0x14c/0x430 net/netlink/af_netlink.c:2550
>> genl_rcv+0x2d/0x40 net/netlink/genetlink.c:1219
>> netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
>> netlink_unicast+0x878/0xb20 net/netlink/af_netlink.c:1344
>> netlink_sendmsg+0x897/0xd70 net/netlink/af_netlink.c:1894
>> sock_sendmsg_nosec net/socket.c:727 [inline]
>> __sock_sendmsg net/socket.c:742 [inline]
>> ____sys_sendmsg+0xa39/0xbf0 net/socket.c:2592
>> ___sys_sendmsg+0x121/0x1c0 net/socket.c:2646
>> __sys_sendmsg+0x155/0x200 net/socket.c:2678
>> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>> do_syscall_64+0x5f/0x350 arch/x86/entry/syscall_64.c:94
>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> </TASK>
>>
>> Fixes: fe67b063f687 ("net/handshake: convert handshake_nl_accept_doit()
>> to FD_PREPARE()")
>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>> ---
>> net/handshake/netlink.c | 35 ++++++++++++++++++-----------------
>> 1 file changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
>> index 1d33a4675a48..cdaea8b8d004 100644
>> --- a/net/handshake/netlink.c
>> +++ b/net/handshake/netlink.c
>> @@ -106,25 +106,26 @@ int handshake_nl_accept_doit(struct sk_buff *skb,
>> struct genl_info *info)
>>
>> err = -EAGAIN;
>> req = handshake_req_next(hn, class);
>> - if (req) {
>> - sock = req->hr_sk->sk_socket;
>> -
>> - FD_PREPARE(fdf, O_CLOEXEC, sock->file);
>> - if (fdf.err) {
>> - err = fdf.err;
>> - goto out_complete;
>> - }
>> -
>> - get_file(sock->file); /* FD_PREPARE() consumes a reference. */
>> - err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
>> - if (err)
>> - goto out_complete; /* Automatic cleanup handles fput */
>> -
>> - trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
>> - fd_publish(fdf);
>> - return 0;
>> + if (!req)
>> + goto out_status;
>> +
>> + sock = req->hr_sk->sk_socket;
>> +
>> + FD_PREPARE(fdf, O_CLOEXEC, sock->file);
>> + if (fdf.err) {
>> + err = fdf.err;
>> + goto out_complete;
>> }
>>
>> + get_file(sock->file); /* FD_PREPARE() consumes a reference. */
>> + err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
>> + if (err)
>> + goto out_complete; /* Automatic cleanup handles fput */
>> +
>> + trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
>> + fd_publish(fdf);
>> + return 0;
>> +
>> out_complete:
>> handshake_complete(req, -EIO, NULL);
>> out_status:
>> --
>> 2.34.1
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/kernel-tls-handshake/aScekpuOYHRM9uOd@morisot.1015granger.net/T/#m7cfa5c11efc626d77622b2981591197a2acdd65e
Thanks for the reminder. I hadn't noticed this email before.
I will add this in v2 patch.
------
Best regards
Wang Liang
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-09 11:58 [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete() Wang Liang
2025-12-09 14:39 ` Chuck Lever
@ 2025-12-09 18:06 ` Simon Horman
2025-12-10 2:51 ` Wang Liang
2025-12-10 1:36 ` kernel test robot
2 siblings, 1 reply; 7+ messages in thread
From: Simon Horman @ 2025-12-09 18:06 UTC (permalink / raw)
To: Wang Liang
Cc: chuck.lever, davem, edumazet, kuba, pabeni, brauner,
kernel-tls-handshake, netdev, linux-kernel, yuehaibing,
zhangchangzhong
On Tue, Dec 09, 2025 at 07:58:52PM +0800, Wang Liang wrote:
> A null pointer dereference in handshake_complete() was observed [1].
>
> When handshake_req_next() return NULL in handshake_nl_accept_doit(),
> function handshake_complete() will be called unexpectedly which triggers
> this crash. Fix it by goto out_status when req is NULL.
>
> [1]
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
> RIP: 0010:handshake_complete+0x36/0x2b0 net/handshake/request.c:288
> Call Trace:
> <TASK>
> handshake_nl_accept_doit+0x32d/0x7e0 net/handshake/netlink.c:129
> genl_family_rcv_msg_doit+0x204/0x300 net/netlink/genetlink.c:1115
> genl_family_rcv_msg+0x436/0x670 net/netlink/genetlink.c:1195
> genl_rcv_msg+0xcc/0x170 net/netlink/genetlink.c:1210
> netlink_rcv_skb+0x14c/0x430 net/netlink/af_netlink.c:2550
> genl_rcv+0x2d/0x40 net/netlink/genetlink.c:1219
> netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
> netlink_unicast+0x878/0xb20 net/netlink/af_netlink.c:1344
> netlink_sendmsg+0x897/0xd70 net/netlink/af_netlink.c:1894
> sock_sendmsg_nosec net/socket.c:727 [inline]
> __sock_sendmsg net/socket.c:742 [inline]
> ____sys_sendmsg+0xa39/0xbf0 net/socket.c:2592
> ___sys_sendmsg+0x121/0x1c0 net/socket.c:2646
> __sys_sendmsg+0x155/0x200 net/socket.c:2678
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x5f/0x350 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> </TASK>
>
> Fixes: fe67b063f687 ("net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE()")
> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
> net/handshake/netlink.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 1d33a4675a48..cdaea8b8d004 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -106,25 +106,26 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>
> err = -EAGAIN;
> req = handshake_req_next(hn, class);
> - if (req) {
> - sock = req->hr_sk->sk_socket;
> -
> - FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> - if (fdf.err) {
> - err = fdf.err;
> - goto out_complete;
> - }
> -
> - get_file(sock->file); /* FD_PREPARE() consumes a reference. */
> - err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
> - if (err)
> - goto out_complete; /* Automatic cleanup handles fput */
> -
> - trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
> - fd_publish(fdf);
> - return 0;
> + if (!req)
> + goto out_status;
> +
> + sock = req->hr_sk->sk_socket;
> +
> + FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> + if (fdf.err) {
> + err = fdf.err;
> + goto out_complete;
> }
>
> + get_file(sock->file); /* FD_PREPARE() consumes a reference. */
> + err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
> + if (err)
> + goto out_complete; /* Automatic cleanup handles fput */
> +
> + trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
> + fd_publish(fdf);
> + return 0;
> +
> out_complete:
> handshake_complete(req, -EIO, NULL);
> out_status:
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!
--
pw-bot: changes-requested
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-09 18:06 ` Simon Horman
@ 2025-12-10 2:51 ` Wang Liang
2025-12-10 10:08 ` Simon Horman
0 siblings, 1 reply; 7+ messages in thread
From: Wang Liang @ 2025-12-10 2:51 UTC (permalink / raw)
To: Simon Horman, Chuck Lever
Cc: chuck.lever, davem, edumazet, kuba, pabeni, brauner,
kernel-tls-handshake, netdev, linux-kernel, yuehaibing,
zhangchangzhong
在 2025/12/10 2:06, Simon Horman 写道:
> On Tue, Dec 09, 2025 at 07:58:52PM +0800, Wang Liang wrote:
>> A null pointer dereference in handshake_complete() was observed [1].
>>
>> When handshake_req_next() return NULL in handshake_nl_accept_doit(),
>> function handshake_complete() will be called unexpectedly which triggers
>> this crash. Fix it by goto out_status when req is NULL.
>>
>> [1]
>> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
>> RIP: 0010:handshake_complete+0x36/0x2b0 net/handshake/request.c:288
>> Call Trace:
>> <TASK>
>> handshake_nl_accept_doit+0x32d/0x7e0 net/handshake/netlink.c:129
>> genl_family_rcv_msg_doit+0x204/0x300 net/netlink/genetlink.c:1115
>> genl_family_rcv_msg+0x436/0x670 net/netlink/genetlink.c:1195
>> genl_rcv_msg+0xcc/0x170 net/netlink/genetlink.c:1210
>> netlink_rcv_skb+0x14c/0x430 net/netlink/af_netlink.c:2550
>> genl_rcv+0x2d/0x40 net/netlink/genetlink.c:1219
>> netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
>> netlink_unicast+0x878/0xb20 net/netlink/af_netlink.c:1344
>> netlink_sendmsg+0x897/0xd70 net/netlink/af_netlink.c:1894
>> sock_sendmsg_nosec net/socket.c:727 [inline]
>> __sock_sendmsg net/socket.c:742 [inline]
>> ____sys_sendmsg+0xa39/0xbf0 net/socket.c:2592
>> ___sys_sendmsg+0x121/0x1c0 net/socket.c:2646
>> __sys_sendmsg+0x155/0x200 net/socket.c:2678
>> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>> do_syscall_64+0x5f/0x350 arch/x86/entry/syscall_64.c:94
>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> </TASK>
>>
>> Fixes: fe67b063f687 ("net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE()")
>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>> ---
>> net/handshake/netlink.c | 35 ++++++++++++++++++-----------------
>> 1 file changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
>> index 1d33a4675a48..cdaea8b8d004 100644
>> --- a/net/handshake/netlink.c
>> +++ b/net/handshake/netlink.c
>> @@ -106,25 +106,26 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>>
>> err = -EAGAIN;
>> req = handshake_req_next(hn, class);
>> - if (req) {
>> - sock = req->hr_sk->sk_socket;
>> -
>> - FD_PREPARE(fdf, O_CLOEXEC, sock->file);
>> - if (fdf.err) {
>> - err = fdf.err;
>> - goto out_complete;
>> - }
>> -
>> - get_file(sock->file); /* FD_PREPARE() consumes a reference. */
>> - err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
>> - if (err)
>> - goto out_complete; /* Automatic cleanup handles fput */
>> -
>> - trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
>> - fd_publish(fdf);
>> - return 0;
>> + if (!req)
>> + goto out_status;
>> +
>> + sock = req->hr_sk->sk_socket;
>> +
>> + FD_PREPARE(fdf, O_CLOEXEC, sock->file);
>> + if (fdf.err) {
>> + err = fdf.err;
>> + goto out_complete;
>> }
>>
>> + get_file(sock->file); /* FD_PREPARE() consumes a reference. */
>> + err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
>> + if (err)
>> + goto out_complete; /* Automatic cleanup handles fput */
>> +
>> + trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
>> + fd_publish(fdf);
>> + return 0;
>> +
>> out_complete:
>> handshake_complete(req, -EIO, NULL);
>> out_status:
> 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()?
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-10 2:51 ` Wang Liang
@ 2025-12-10 10:08 ` Simon Horman
0 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2025-12-10 10:08 UTC (permalink / raw)
To: Wang Liang
Cc: Chuck Lever, chuck.lever, davem, edumazet, kuba, pabeni, brauner,
kernel-tls-handshake, netdev, linux-kernel, yuehaibing,
zhangchangzhong
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
2025-12-09 11:58 [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete() Wang Liang
2025-12-09 14:39 ` Chuck Lever
2025-12-09 18:06 ` Simon Horman
@ 2025-12-10 1:36 ` kernel test robot
2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2025-12-10 1:36 UTC (permalink / raw)
To: Wang Liang, chuck.lever, davem, edumazet, kuba, pabeni, horms,
brauner
Cc: llvm, oe-kbuild-all, kernel-tls-handshake, netdev, linux-kernel,
yuehaibing, zhangchangzhong, wangliang74
Hi Wang,
kernel test robot noticed the following build errors:
[auto build test ERROR on net/main]
url: https://github.com/intel-lab-lkp/linux/commits/Wang-Liang/net-handshake-Fix-null-ptr-deref-in-handshake_complete/20251209-194006
base: net/main
patch link: https://lore.kernel.org/r/20251209115852.3827876-1-wangliang74%40huawei.com
patch subject: [PATCH net] net/handshake: Fix null-ptr-deref in handshake_complete()
config: arm-mps2_defconfig (https://download.01.org/0day-ci/archive/20251210/202512100952.cr9q1lGr-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 6ec8c4351cfc1d0627d1633b02ea787bd29c77d8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251210/202512100952.cr9q1lGr-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512100952.cr9q1lGr-lkp@intel.com/
All errors (new ones prefixed by >>):
>> 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);
| ^
3 errors generated.
vim +110 net/handshake/netlink.c
89
90 int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
91 {
92 struct net *net = sock_net(skb->sk);
93 struct handshake_net *hn = handshake_pernet(net);
94 struct handshake_req *req = NULL;
95 struct socket *sock;
96 int class, err;
97
98 err = -EOPNOTSUPP;
99 if (!hn)
100 goto out_status;
101
102 err = -EINVAL;
103 if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_ACCEPT_HANDLER_CLASS))
104 goto out_status;
105 class = nla_get_u32(info->attrs[HANDSHAKE_A_ACCEPT_HANDLER_CLASS]);
106
107 err = -EAGAIN;
108 req = handshake_req_next(hn, class);
109 if (!req)
> 110 goto out_status;
111
112 sock = req->hr_sk->sk_socket;
113
114 FD_PREPARE(fdf, O_CLOEXEC, sock->file);
115 if (fdf.err) {
116 err = fdf.err;
117 goto out_complete;
118 }
119
120 get_file(sock->file); /* FD_PREPARE() consumes a reference. */
121 err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
122 if (err)
123 goto out_complete; /* Automatic cleanup handles fput */
124
125 trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
126 fd_publish(fdf);
127 return 0;
128
129 out_complete:
130 handshake_complete(req, -EIO, NULL);
131 out_status:
132 trace_handshake_cmd_accept_err(net, req, NULL, err);
133 return err;
134 }
135
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread