Re: [PATCH v2 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

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

Pre-existing issues:
- [Critical] Race condition between `isotp_release()` and netdevice unregistration skips CAN filter unregistration, leading to a permanent memory leak and potential use-after-free.
--

--- Patch [1]: [PATCH v2 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit 554fcaa84431e4410ac9203b4f4a5bfeadc94ccf
Author: Oliver Hartkopp <[email protected]>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

This commit makes isotp_release() unconditionally call synchronize_rcu()
before cancelling timers. This closes a race where a concurrent
NETDEV_UNREGISTER could clear so->bound without waiting for in-flight
isotp_rcv() callers.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between
isotp_release() and netdevice unregistration that can skip CAN filter
unregistration?

If unregister_netdevice_many() unlists the netdevice from net->dev_by_index
using unlist_netdevice(), and isotp_release() runs immediately after this but
before the NETDEV_UNREGISTER notifier is dispatched:

isotp_release() {
    ...
    list_del(&so->notifier);
    ...
    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {
        ...
    }
}

Since the device is unlisted, this returns NULL, causing isotp_release() to
silently skip unregistering the CAN RX filters.

When the NETDEV_UNREGISTER notifier eventually runs in isotp_notifier(), it
misses the socket because it was already unlinked from isotp_notifier_list.

Could this permanently leak the CAN filters with a dangling pointer to the
freed socket, leading to a use-after-free? Can this be triggered by an
unprivileged user in a user namespace with CAP_NET_ADMIN by repeatedly
creating and destroying vcan interfaces while simultaneously creating and
closing ISOTP sockets?

> -				synchronize_rcu();
>  			}
>  		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1