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

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

New issues:
- [Medium] Unconditional `synchronize_rcu()` in `isotp_release()` can be triggered by unprivileged users via unbound sockets.

Pre-existing issues:
- [High] Concurrent isotp_sendmsg() callers can forcibly abort another thread's active transmission upon receiving a signal.
--

--- Patch [1]: [PATCH v9 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit d8a6dd60eabcde6d6f502a2471ec7e04cf918503
Author: Oliver Hartkopp <[email protected]>

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

This patch makes isotp_release() always call synchronize_rcu() before
cancelling timers to close a race condition when a concurrent
NETDEV_UNREGISTER clears so->bound.

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);
> -				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: Medium]
Could this unconditional synchronize_rcu() be triggered rapidly by
unprivileged users?

If an unprivileged user creates an unbound CAN_ISOTP socket and closes it
immediately, it appears it will now hit this synchronize_rcu() in
isotp_release() because so->bound is no longer checked. 

Might a rapid loop of socket creation and destruction allow local
unprivileged users to degrade system performance by constantly blocking for
RCU grace periods?


This is a pre-existing issue, but I noticed a potential race condition in
isotp_sendmsg() when handling signals.

[Severity: High]
Can a thread interrupted by a signal forcibly abort another thread's active
transmission?

If multiple threads call isotp_sendmsg() on the same socket, the function
uses cmpxchg() to gain exclusivity over the TX state machine. If a thread
fails the cmpxchg and waits in wait_event_interruptible(), but is then
interrupted by a signal, it jumps to err_event_drop:

net/can/isotp.c:isotp_sendmsg() {
...
err_event_drop:
        /* got signal: force tx state machine to be idle */
        so->tx.state = ISOTP_IDLE;
        hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);
...
}

Does this forcible state reset to ISOTP_IDLE and timer cancellation happen
even though the interrupted thread does not own the state machine? This
appears to abort the active transmission and could allow a third thread to
pass the cmpxchg, leading to concurrent execution of isotp_sendmsg() on the
same so->tx variables.

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