Re: [PATCH net] can: isotp: take rtnl_lock() before leaving the notifier list

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hello Norbert,

many thanks for your patch and the analysis of the unremoved filter 
lists in the case of moving a CAN device to another namespace.

But I don't think that moving rtnl_lock() up so that it covers a busy 
loop including a schedule_timeout_uninterruptible(1) wait is not a nice 
move for other rtnl_lock() users.

Focussing on the removal of the correct filter lists when the namespace 
is changed away from the socket's namespace I would propose this small 
change:

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 155530aedce2..0835a4758a72 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1490,15 +1490,15 @@ static int isotp_release(struct socket *sock)
         /* remove current filters & unregister
          * tracked reference so->dev is taken at bind() time with rtnl_lock
          */
         if (so->bound && so->dev) {
                 if (isotp_register_rxid(so))
-                       can_rx_unregister(net, so->dev, so->rxid,
+                       can_rx_unregister(dev_net(so->dev), so->dev, 
so->rxid,
                                           SINGLE_MASK(so->rxid),
                                           isotp_rcv, sk);

-               can_rx_unregister(net, so->dev, so->txid,
+               can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
                                   SINGLE_MASK(so->txid),
                                   isotp_rcv_echo, sk);
                 netdev_put(so->dev, &so->dev_tracker);
         }

@@ -1846,13 +1846,10 @@ static int isotp_getsockopt(struct socket *sock, 
int level, int optname,
  static void isotp_notify(struct isotp_sock *so, unsigned long msg,
                          struct net_device *dev)
  {
         struct sock *sk = &so->sk;

-       if (!net_eq(dev_net(dev), sock_net(sk)))
-               return;
-
         if (so->dev != dev)
                 return;

         switch (msg) {
         case NETDEV_UNREGISTER:


Can you give it a try with your KASAN setup and maybe also ask opus 
about my idea?

Many thanks,
Oliver

On 31.08.26 10:30, Norbert Szetei wrote:
> isotp_release() removes the socket from isotp_notifier_list before it
> takes rtnl_lock(). The netdev notifier chain runs under RTNL, so a
> socket that leaves the list in that window is skipped by isotp_notify()
> and has to unregister its own CAN filters.
> 
> It cannot always do that. isotp_release() passes sock_net(sk) to
> can_rx_unregister(), which returns early when that netns no longer
> matches dev_net(dev), before the receiver list is searched and before
> the "receive list entry not found" warning. Once the bound device has
> been moved to another netns the filters are removed zero times, and
> can_rx_register() stores rcv->sk without taking a reference, so the
> receivers left in the device's dev_rcv_lists point at the freed socket
> and travel with the device into the new netns.
> 
>    BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0
>    Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578
>     can_rcv_filter+0x4af/0x8c0
>     can_receive+0x28d/0x3c0
>     can_rcv+0x2a9/0x310
>     __netif_receive_skb_one_core+0x21a/0x260
>     process_backlog+0x210/0x760
> 
> Take rtnl_lock() before removing the socket from the notifier list, so
> that isotp_release() and isotp_notify() cannot both skip the removal.
> 
> Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> ---
> Reproducer available on request.
> 
>   net/can/isotp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 155530aedce2..8ca75d30360c 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1475,6 +1475,8 @@ static int isotp_release(struct socket *sock)
>   	/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */
>   	wake_up_interruptible(&so->wait);
>   
> +	rtnl_lock();
> +
>   	spin_lock(&isotp_notifier_lock);
>   	while (isotp_busy_notifier == so) {
>   		spin_unlock(&isotp_notifier_lock);
> @@ -1484,7 +1486,6 @@ static int isotp_release(struct socket *sock)
>   	list_del(&so->notifier);
>   	spin_unlock(&isotp_notifier_lock);
>   
> -	rtnl_lock();
>   	lock_sock(sk);
>   
>   	/* remove current filters & unregister