Re: [PATCH] can: notifiers: remove CAN filters independent from namespace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the namespace equality check in `bcm_notify` exposes BCM sockets to cross-namespace `ifindex` collisions. Unprivileged users can exploit this to disrupt BCM sockets in the host or other namespaces by spoofing `NETDEV_UNREGISTER` and `NETDEV_DOWN` events.
--

commit e5738773c45b8a30848fd2bb47977b2587f26903
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: notifiers: remove CAN filters independent from namespace
    
This commit removes the namespace equality check in the netdev notifiers
for BCM, ISOTP, and RAW CAN sockets. The goal is to ensure CAN filters
are correctly removed when a device is unregistered, even if the socket
and device network namespaces differ due to a namespace change. It also
updates the can_rx_unregister() calling sites to retrieve the namespace
from the device instead of the socket.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3d637a1e0ac1a..aaa5163c46cc2 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1794,9 +1794,6 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  	struct bcm_op *op;
>  	int notify_enodev = 0;
>  
> -	if (!net_eq(dev_net(dev), sock_net(sk)))
> -		return;
> -

[Severity: High]
With this namespace equality check removed, are BCM sockets now exposed to
cross-namespace ifindex collisions?

Because ifindex is only unique per namespace, an unprivileged user could create
a virtual CAN device in a new network namespace with an ifindex matching a
target BCM socket in another namespace.

When this spoofed device is unregistered or brought down, the events proceed
into the rest of bcm_notify() which compares the integer ifindex instead of
a device pointer:

net/can/bcm.c:bcm_notify() {
    ...
    case NETDEV_UNREGISTER:
        ...
        /* ANYDEV ops */
        if (!op->ifindex) {
            spin_lock_bh(&op->bcm_rx_update_lock);
            if (op->if_detected == dev->ifindex)
                op->if_detected = 0;
            ...
        /* tx_ops */
        list_for_each_entry(op, &bo->tx_ops, list)
            if (op->ifindex == dev->ifindex)
                hrtimer_cancel(&op->timer);
        ...
        /* bound device */
        if (bo->bound && bo->ifindex == dev->ifindex) {
            ...
            WRITE_ONCE(bo->bound, 0);
            bo->ifindex = 0;
            notify_enodev = 1;
        }
    ...
    case NETDEV_DOWN:
        if (bo->bound && bo->ifindex == dev->ifindex) {
            sk->sk_err = ENETDOWN;
            ...
}

Could this allow an attacker to falsely clear ANYDEV claims, cancel active
transmission timers, forcibly unbind the socket (ENODEV), or inject ENETDOWN
errors into victim BCM sockets across namespaces?

>  	switch (msg) {
>  
>  	case NETDEV_UNREGISTER:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831185654.16701-1-socketcan@hartkopp.net?part=1