Re: [PATCH v6 1/5] 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:
- [High] Race conditions between isotp_release/isotp_bind and NETDEV_UNREGISTER cause CAN filters to be leaked, leading to a Use-After-Free of the socket.
--

--- Patch [1]: [PATCH v6 1/5] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit 9e3b5ef819dd616609004db7956c6318a9ae781a
Author: Oliver Hartkopp <[email protected]>

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

isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister()
and clears so->bound without waiting for a grace period. isotp_release()
uses so->bound to decide whether it needs to call synchronize_rcu()
before cancelling so->rxtimer, so when NETDEV_UNREGISTER runs first it
skips that synchronize_rcu() and can cancel the timer while an
in-flight isotp_rcv() is still executing and about to re-arm it via
isotp_send_fc(), leading to a use-after-free timer callback on the
freed socket.

sakisho-bot remarked a problem with rtnl_lock held in isotp_notify(),
therefore make isotp_release() always call synchronize_rcu() before
cancelling the timers, regardless of so->bound. This still closes the
original race (isotp_notify() clearing so->bound without waiting for
in-flight isotp_rcv() callers before isotp_release() cancels the RX
timer) without adding any RCU wait to the netdevice notifier path.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
Closes: https://lore.kernel.org/linux-can/[email protected]/
Reported-by: Nico Yip <[email protected]> (ZDI-CAN-31764)

> 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);
> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +

[Severity: High]
This is a pre-existing issue, but can race conditions between isotp_release()
or isotp_bind() and NETDEV_UNREGISTER cause CAN filters to be leaked,
potentially leading to a use-after-free of the socket?

If a user creates a CAN interface, binds an ISOTP socket, and concurrently
unregisters the interface while closing the socket, isotp_release() removes
the socket from the notifier list:

net/can/isotp.c:isotp_release() {
        ...
        list_del(&so->notifier);
        spin_unlock(&isotp_notifier_lock);
        
        lock_sock(sk);
        ...
}

This blinds the NETDEV_UNREGISTER notifier. If isotp_release() executes before
isotp_notify(), the notifier skips the socket. When isotp_release() then tries
to clean up the filters, if the netdevice is already unlisted, it might skip
the can_rx_unregister() call. The socket would then be freed, but the active
CAN filter could still point to it.

A similar window exists in isotp_bind(), where the filters are registered
before so->ifindex is updated:

net/can/isotp.c:isotp_bind() {
        ...
        if (isotp_register_rxid(so))
                can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), ...);
        
        can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id), ...);
        dev_put(dev);
        
        /* switch to new settings */
        so->ifindex = ifindex;
        so->rxid = rx_id;
        ...
}

If a concurrent NETDEV_UNREGISTER runs during this window, isotp_notify()
ignores the socket because it locklessly checks if (so->ifindex != dev->ifindex),
bypassing the unregister and leaking the filter when the socket is later
released.

Could this allow a local unprivileged user utilizing virtual CAN (vcan)
devices to trigger a use-after-free?

>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

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