[PATCH net v1] net/mctp: publish the mctp_dev only after it is initialised

[PATCH net v1] net/mctp: publish the mctp_dev only after it is initialised

[Binbin Deng]

From: Binbin Deng <dengbinbin21@nudt.edu.cn>

mctp_add_dev() links the new mctp_dev into dev->mctp_ptr before mdev->dev
is assigned, so a reader that picks the device up in that window
dereferences a net_device pointer that is still NULL:

KASAN: null-ptr-deref in range [0x00000000000000b0-0x00000000000000b7]
Call Trace:
 <TASK>
 ? __pfx_mctp_sendmsg (./include/linux/sockptr.h:49)
 ? __pfx_mctp_dst_output (net/mctp/route.c:41)
 ? selinux_socket_sendmsg (security/selinux/hooks.c:5278)
 ____sys_sendmsg (net/socket.c:775 (discriminator 1) net/socket.c:790
 (discriminator 1) net/socket.c:2684 (discriminator 1))
 ? __pfx_____sys_sendmsg (net/socket.c:1131)
 ? __pfx_copy_msghdr_from_user (net/socket.c:2590)
 ? update_cfs_rq_load_avg (kernel/sched/fair.c:5478)
 ___sys_sendmsg (net/socket.c:2738)
 ? __pfx____sys_sendmsg (net/socket.c:2625)
 ? perf_event_task_tick (./include/linux/rcupdate.h:838)
 ? sched_tick (kernel/sched/core.c:5818)
 ? clockevents_program_event (kernel/time/clockevents.c:372)
 ? fdget (./include/linux/rcupdate.h:873 fs/file.c:1100)
 __sys_sendmsg (net/socket.c:2770)
 ? __pfx___sys_sendmsg (net/socket.c:2751)
 do_syscall_64 (arch/x86/entry/syscall_64.c:63)
 entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:121)
 </TASK>

Fix by publishing only once the object is fully initialised:
__mctp_dev_get() runs under rcu_read_lock() only and hands
this object to readers as soon as mctp_ptr is assigned, so
they must never observe mdev->dev == NULL.

Fixes: 583be982d934 ("mctp: Add device handling and netlink interface")
Signed-off-by: Binbin Deng <dengbinbin21@nudt.edu.cn>
---
 net/mctp/device.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/mctp/device.c b/net/mctp/device.c
index 822120e860c8..b76715ea50e1 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -344,13 +344,18 @@ static struct mctp_dev *mctp_add_dev(struct net_device *dev)
 
 	mdev->net = mctp_default_net(dev_net(dev));
 
-	/* associate to net_device */
 	refcount_set(&mdev->refs, 1);
-	rcu_assign_pointer(dev->mctp_ptr, mdev);
-
 	dev_hold(dev);
 	mdev->dev = dev;
 
+	/*
+	 * Associate to net_device.  Publish only once the object is fully
+	 * initialised: __mctp_dev_get() runs under rcu_read_lock() only and
+	 * hands this object to readers as soon as mctp_ptr is assigned, so
+	 * they must never observe mdev->dev == NULL.
+	 */
+	rcu_assign_pointer(dev->mctp_ptr, mdev);
+
 	return mdev;
 }
 
-- 
2.43.0

Re: [PATCH net v1] net/mctp: publish the mctp_dev only after it is initialised

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message presents an unverifiable/self-inconsistent
  KASAN report as the justification for the fix.

Pre-existing issues:
- [Medium] mctp_register_netdevice() (net/mctp/device.c:483-497) calls
  mctp_add_dev(), which kzalloc()s a struct mctp_dev with refcount 1 and…

--

net/mctp: publish the mctp_dev only after it is initialised

The patch moves the rcu_assign_pointer(dev->mctp_ptr, mdev) store in
mctp_add_dev() to after mdev->dev is set, so RCU readers that pick the
device up via __mctp_dev_get() can no longer observe mdev->dev == NULL.
The reordering itself looks correct.

A couple of questions below, one about the evidence in the commit message
and one about a pre-existing rollback gap in the same allocation path.

> net/mctp: publish the mctp_dev only after it is initialised
>
> mctp_add_dev() links the new mctp_dev into dev->mctp_ptr before mdev->dev
> is assigned, so a reader that picks the device up in that window
> dereferences a net_device pointer that is still NULL:
>
> KASAN: null-ptr-deref in range [0x00000000000000b0-0x00000000000000b7]
> Call Trace:
>  <TASK>
>  ? __pfx_mctp_sendmsg (./include/linux/sockptr.h:49)
>  ? __pfx_mctp_dst_output (net/mctp/route.c:41)
>  ? selinux_socket_sendmsg (security/selinux/hooks.c:5278)
>  ____sys_sendmsg (net/socket.c:775 ...)
>  ...
>  do_syscall_64 (arch/x86/entry/syscall_64.c:63)
>  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:121)
>  </TASK>

[Severity: Medium]
Can this trace actually correspond to the race being described?

There is no faulting instruction line in the report (no "Read of size N at
addr ..." and no RIP:/top frame), and the only MCTP entries are unreliable
"?" frames that resolve to kcfi padding symbols:

     ? __pfx_mctp_sendmsg (./include/linux/sockptr.h:49)
     ? __pfx_mctp_dst_output (net/mctp/route.c:41)

A __pfx_foo symbol is the 16-byte padding placed before foo, so it can
never be a live call site. The remaining "?" entries are stale scheduler
and perf frames (update_cfs_rq_load_avg, sched_tick,
clockevents_program_event, perf_event_task_tick), leaving only
____sys_sendmsg/___sys_sendmsg/__sys_sendmsg/do_syscall_64 as reliable
frames.

For the window described, the dereference of the half-initialised mdev
happens here:

net/mctp/route.c:mctp_dst_output() {
    ...
	skb->dev = dst->dev->dev;
    ...
}

Would a genuine splat not list mctp_dst_output() and the following xmit
frame as reliable frames?

The window itself does look real: register_netdevice() calls
list_netdevice(dev) before call_netdevice_notifiers(NETDEV_REGISTER, dev),
so while mctp_register()->mctp_add_dev() runs from the notifier, the
net_device is already reachable through dev_get_by_index_rcu(), and a
concurrent sendmsg with MCTP_ADDR_EXT can reach
mctp_dst_from_extaddr()->__mctp_dev_get().

Could the changelog describe that reachability path instead, and add the
provenance (Reported-by:/Closes:/reproducer, or a statement that this was
found by code inspection)? As it stands the claimed crash cannot be
validated by reviewers or stable maintainers.

> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 822120e860c82..b76715ea50e14 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -344,13 +344,18 @@ static struct mctp_dev *mctp_add_dev(struct net_device *dev)
>  
>  	mdev->net = mctp_default_net(dev_net(dev));
>  
> -	/* associate to net_device */
>  	refcount_set(&mdev->refs, 1);
> -	rcu_assign_pointer(dev->mctp_ptr, mdev);
> -
>  	dev_hold(dev);
>  	mdev->dev = dev;
>  
> +	/*
> +	 * Associate to net_device.  Publish only once the object is fully
> +	 * initialised: __mctp_dev_get() runs under rcu_read_lock() only and
> +	 * hands this object to readers as soon as mctp_ptr is assigned, so
> +	 * they must never observe mdev->dev == NULL.
> +	 */
> +	rcu_assign_pointer(dev->mctp_ptr, mdev);
> +
>  	return mdev;
>  }
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the
allocation done here: does the mctp_dev allocated by mctp_add_dev(), plus
the dev_hold(dev) reference taken next to it, leak when
register_netdevice() fails?

net/mctp/device.c:mctp_register_netdevice() {
	mdev = mctp_add_dev(dev);
	if (IS_ERR(mdev))
		return PTR_ERR(mdev);

	mdev->ops = ops;
	mdev->binding = binding;

	return register_netdevice(dev);
}

register_netdevice() has several exits that return an error without ever
delivering NETDEV_UNREGISTER - dev_get_valid_name() failure,
netdev_name_node_head_alloc() -ENOMEM, ndo_init() failure, the buggy-VLAN
-EINVAL, netdev_do_alloc_pcpu_stats() failure, dev_index_reserve()
failure, NETDEV_POST_INIT notifier failure and netdev_register_kobject()
failure (which only emits NETDEV_PRE_UNINIT):

net/core/dev.c:register_netdevice() {
    ...
err_uninit_notify:
	call_netdevice_notifiers(NETDEV_PRE_UNINIT, dev);
err_ifindex_release:
	dev_index_release(net, dev->ifindex);
    ...
err_free_name:
	netdev_name_node_free(dev->name_node);
	goto out;
}

The only place that drops the mctp_dev is mctp_unregister(), and it only
runs from the NETDEV_UNREGISTER notifier:

net/mctp/device.c:mctp_unregister() {
	RCU_INIT_POINTER(mdev->dev->mctp_ptr, NULL);
    ...
	mctp_dev_put(mdev);
}

All four in-tree MCTP drivers then free the netdev directly, for example:

drivers/net/mctp/mctp-serial.c:mctp_serial_open() {
	rc = mctp_register_netdev(ndev, NULL, MCTP_PHYS_BINDING_SERIAL);
	if (rc)
		goto free_netdev;
}

with reg_state still NETREG_UNINITIALIZED, so free_netdev() frees the
netdev immediately while the mctp_dev is left allocated with a stale
mdev->dev and an unbalanced netdev reference (which also shows up as a
ref_tracker "leaked reference" report with
CONFIG_NET_DEV_REFCNT_TRACKER=y). Would an explicit rollback on the
register_netdevice() error return - RCU_INIT_POINTER(dev->mctp_ptr, NULL)
followed by mctp_dev_put(mdev) - be appropriate, perhaps as a separate
patch?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922125443.133110-1-18983559317%40163.com