Re: [PATCH v4 1/3] can: remove CAN filters independent from namespace

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 1

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).

- [High] bcm_notify() now calls lock_sock(sk) right away for every BCM
  socket on the global bcm_notifier_list.
- [High] Releasing a BCM socket that races with a namespace move of its
  bound device can still leave a receive filter pointing at a freed…
- [Medium] The commit message's account of the bug cannot happen.
- [Low] The commit message presents the isotp_release() change as
  converting two 'missing' call sites.

--

can: remove CAN filters independent from namespace

This patch removes the net_eq(dev_net(dev), sock_net(sk)) early return
from the raw, isotp and bcm netdev notifiers. In bcm.c it adds tracked
device pointers (bo->dev, op->tx_dev) for device comparisons and takes
lock_sock() for all of bcm_notify(). In isotp_release() it passes
dev_net(so->dev) to can_rx_unregister() instead of sock_net(sk).

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/

> When the devices namespace is changed the socket namespace and the device
> namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in
> the CAN protocols netdev notifiers therefore led to skipping the required
> removal of the CAN filters from the (namespace changed) CAN devices.

[Severity: Medium]
Is this description of the bug accurate? In __dev_change_net_namespace(),
NETDEV_UNREGISTER is sent while dev_net(dev) still points to the old
namespace. The namespace is switched only afterwards:

net/core/dev.c:__dev_change_net_namespace() {
    ...
	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
    ...
	dev_net_set(dev, net);
    ...
}

Every bound device is resolved with dev_get_by_index(sock_net(sk), ...).
raw_bind() and isotp_bind() do this under rtnl_lock(). bcm_connect() and
bcm_rx_setup() do it under lock_sock(). So for every socket still on the
notifier list, the removed net_eq() check seems to have always been true
when its device got NETDEV_UNREGISTER.

In raw_notify() and isotp_notify(), the ro->dev != dev and so->dev != dev
checks that follow already made the net_eq() check redundant.

The bug that actually existed appears to be the sock_net(sk) argument in
isotp_release(). That function runs after the socket has left the
notifier list.

Could the commit message describe the real mechanism? As written, it also
justifies the bcm.c rework (bo->dev, op->tx_dev and the unconditional
lock_sock() in bcm_notify()). That rework is needed only because the
net_eq() check was removed, and it is the source of the bcm_notify()
concern below.

> In isotp.c the two missing can_rx_unregister() calling sites are converted
> to use dev_net(dev) instead of sock_net(sk) to get the correct namespace.

[Severity: Low]
This hunk looks like the actual fix for a use-after-free, but the commit
message doesn't say so. Could it describe the race and the use-after-free?

isotp_release() removes the socket from isotp_notifier_list before it
takes rtnl_lock(). If __dev_change_net_namespace() runs in that window,
its NETDEV_UNREGISTER notifier never sees the socket, and the device moves
to the new namespace.

Before this patch, isotp_release() then passed sock_net(sk) to
can_rx_unregister(), which returns early on the namespace mismatch:

net/can/af_can.c:can_rx_unregister() {
    ...
	if (dev && !net_eq(net, dev_net(dev)))
		return;
    ...
}

The isotp_rcv() and isotp_rcv_echo() receivers stay registered with
data == sk, and sock_put(sk) then frees the socket. can_rx_register()
holds no reference on sk, so the next matching frame on the moved device
calls isotp_rcv() on freed memory.

raw_release() already used dev_net(ro->dev) under rtnl_lock(), and bcm
goes through bcm_rx_unreg(), which uses dev_net(dev).

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3d637a1e0ac1a..cd3522ec32c0a 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1792,15 +1833,13 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  {
>  	struct sock *sk = &bo->sk;
>  	struct bcm_op *op;
> -	int notify_enodev = 0;
> +	int sk_err = 0;
>  
> -	if (!net_eq(dev_net(dev), sock_net(sk)))
> -		return;
> +	lock_sock(sk);

[Severity: High]
Can a user confined to their own network namespace now stall rtnl_mutex
here indefinitely?

bcm_notifier() walks every BCM socket on bcm_notifier_list for each CAN
NETDEV_DOWN or NETDEV_UNREGISTER, with rtnl_lock() held. Before this
patch, sockets in a foreign namespace returned before taking any lock,
and NETDEV_DOWN never took lock_sock(). Now lock_sock() is taken on every
BCM socket in the system for both events.

bcm_sendmsg() holds the socket lock across user copies:

bcm_sendmsg()
    lock_sock(sk);
    bcm_tx_setup()
        memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

bcm_rx_setup() has the same pattern.

An unprivileged user can put the frame data on a page whose fault they
control, for example a FUSE-backed mapping inside a user namespace. The
socket lock can then be held for as long as they want.

Any host-side "ip link set can0 down", unplug, unregister or netns move of
any CAN device would then block in bcm_notify() with rtnl_lock() held.
All link, address and route configuration and netns creation or cleanup
would stall behind it.

A similar same-namespace stall was already possible for users who can
create and delete their own vcan. This patch extends it to host devices
and to NETDEV_DOWN.

Could the bo->dev, op->tx_dev and op->rx_reg_dev comparisons be done as a
prefilter, so that lock_sock() is not taken on unrelated sockets? Even
with the rest of the series applied, only the device type check in
bcm_notifier() changes, so this lock_sock() stays unconditional.

>  
>  	switch (msg) {
>  
>  	case NETDEV_UNREGISTER:
> -		lock_sock(sk);

[ ... ]

> @@ -1984,6 +2025,10 @@ static int bcm_release(struct socket *sock)
>  	if (bo->bound) {
>  		WRITE_ONCE(bo->bound, 0);
>  		bo->ifindex = 0;
> +		if (bo->dev) {
> +			netdev_put(bo->dev, &bo->dev_tracker);
> +			bo->dev = NULL;
> +		}
>  	}

[Severity: High]
Can bcm_release() still leave an rx filter pointing at a freed bcm_op if
it races with a namespace move of the bound device?

bcm_release() removes the socket from bcm_notifier_list and then takes
only lock_sock(sk), not rtnl_lock(). For each rx_op with op->rx_reg_dev,
teardown goes through bcm_rx_unreg():

bcm_rx_unreg()
    can_rx_unregister(dev_net(dev), dev, ...)
        if (dev && !net_eq(net, dev_net(dev)))
            return;

Meanwhile __dev_change_net_namespace() runs under rtnl_lock(). Its
NETDEV_UNREGISTER notifier skips this socket because the socket is already
off the list. It then calls dev_net_set(dev, net).

Suppose dev_net_set() lands between the dev_net(dev) read in
bcm_rx_unreg() and the re-read in can_rx_unregister(). The receiver then
stays linked with func == bcm_rx_handler and data == op.

can_rx_unregister() returns void, so bcm_rx_unreg() still clears
op->rx_reg_dev and drops the reference. bcm_release() then frees the op:

bcm_remove_op()
    call_rcu() -> bcm_free_op_work()
        sock_put(op->sk);
        kfree(op);

The next matching frame on the moved device would call bcm_rx_handler()
on freed memory. The window per op is narrow, but it can be retried, and
a socket with many rx_ops widens it.

isotp_release() and raw_release() avoid this by holding rtnl_lock() around
the unregister. Should bcm_release() take rtnl_lock() as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929154125.3298-1-socketcan%40hartkopp.net