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

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

Pre-existing issues:
- [High] CAN filter leak in isotp_release() during concurrent netdevice unregistration leading to Use-After-Free.
- [High] Missing synchronization and state cleanup in isotp_sendmsg error path causes data corruption when racing with CAN echo frames.
--

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

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

This commit adds an unconditional synchronize_rcu() in isotp_release()
before cancelling timers. It addresses a race condition where a
concurrent NETDEV_UNREGISTER could cause a use-after-free in the timer
callback on the freed socket.

Closes: 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: High]
This is a pre-existing issue, but is there a potential CAN filter leak here
that could lead to a use-after-free?

If a user closes the ISOTP socket exactly when the bound netdevice is being
unregistered, unlist_netdevice(dev) is called before the NETDEV_UNREGISTER
notifiers.

If isotp_release() runs in this window, it removes the socket from
isotp_notifier_list and calls dev_get_by_index(). Since the device is
unlisted, dev_get_by_index() returns NULL, causing isotp_release() to skip
can_rx_unregister().

Because the filter is left active, any matching CAN frames subsequently
processed by the unregistering device might trigger isotp_rcv() with the
freed socket pointer. Does the newly added synchronize_rcu() below prevent
this if the filter is never actually unregistered?

> -				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();
> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

[Severity: High]
This is a pre-existing issue, but does the error path in isotp_sendmsg()
properly clean up the echo state when interrupted by a signal?

If a user initiates a blocking transfer with wait_tx_done enabled, and it
is interrupted by a signal, the err_event_drop path forcefully sets
so->tx.state = ISOTP_IDLE without clearing so->cfecho or waiting for the
in-flight CAN hardware echo:

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);
[ ... ]
}

When the background echo frame arrives, isotp_rcv_echo() matches the stale
so->cfecho and unconditionally continues transmission by reading from
so->tx.buf and incrementing so->tx.idx:

net/can/isotp.c:isotp_rcv_echo() {
[ ... ]
        if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
                return;
[ ... ]
        if (!so->tx_gap) {
                /* enable echo timeout handling */
                hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
                              HRTIMER_MODE_REL_SOFT);
                isotp_send_cframe(so);
                return;
        }
[ ... ]
}

Concurrently, the user can call sendmsg() again since the state is IDLE,
overwriting the buffer and lengths without locks. Could this time-of-check
to time-of-use race cause the background softirq to read corrupted indices
and result in out-of-bounds memory accesses?

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