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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Race condition between isotp_sendmsg() error paths and isotp_rcv_fc() allows leaking uninitialized kernel heap memory over the CAN bus.
- [High] CAN filter leak due to a race condition between socket release and netdevice unregistration.
- [High] State Machine Leak and Cross-Interface Data Leakage on NETDEV_UNREGISTER.
- [High] Lockless updates to the TX state machine in isotp_sendmsg() race with concurrent CAN frame reception.
- [Medium] Missing Error Handling in isotp_bind() ignores can_rx_register() failures.
--

--- Patch [1]: [PATCH v8 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit 6e5426def22a6e6426722ba05c1e7413dd0ccd86
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

isotp_notify() unregisters the (RCU) CAN filters and clears so->bound
without waiting for a grace period. This commit updates isotp_release() to
unconditionally call synchronize_rcu() before cancelling timers, fixing a
race condition where NETDEV_UNREGISTER could lead to a use-after-free timer
callback on the freed socket.

Link: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/

> 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();
>  			}
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does a race condition exist between socket
release and netdevice unregistration that can leak filters?

If isotp_release() runs during the window where unregister_netdevice_many()
has unlisted the device from the IDR but before notifiers are invoked:

net/can/isotp.c:isotp_release() {
    ...
    list_del(&so->notifier);
    ...
    dev = dev_get_by_index(net, so->ifindex);
    ...
}

Because the socket is unlinked from the notifier list, the notifier won't
clean it up. However, dev_get_by_index() returns NULL since the device is
unlisted, causing isotp_release() to silently skip calling
can_rx_unregister() and permanently leak the receiver memory.

> +	/* 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 isn't a bug introduced by this patch, but does the NETDEV_UNREGISTER
handler fail to stop active state machines?

When the unregister event is handled:

net/can/isotp.c:isotp_notify() {
    ...
    case NETDEV_UNREGISTER:
        lock_sock(sk);
        /* remove current filters & unregister */
    ...
}

The code clears so->bound and so->ifindex but omits explicitly aborting
active state machines (so->tx.state) or cancelling tx timers. If a thread
is blocked in isotp_sendmsg(), it remains hung until a timer expires.

Additionally, if userspace calls bind() on a new interface, the state isn't
reset, and when the active timer fires, it could transmit remaining frames
of the old PDU out of the newly bound network interface.

[Severity: Medium]
This is a pre-existing issue, but does isotp_bind() ignore filter
registration failures?

net/can/isotp.c:isotp_bind() {
    ...
    if (isotp_register_rxid(so))
        can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
                        isotp_rcv, sk, "isotp", sk);
    ...
}

If can_rx_register() fails with -ENOMEM, isotp_bind() ignores the return
value, sets so->bound = 1, and returns 0. The socket then silently fails to
receive or echo frames, and later isotp_release() will trigger a warning
splat when attempting to unregister the missing filter.

[Severity: High]
This isn't a bug introduced by this patch, but is there a race with
concurrent CAN frame reception when updating the state locklessly?

net/can/isotp.c:isotp_sendmsg() {
    ...
    so->tx.state = ISOTP_WAIT_FIRST_FC;
    ...
}

If a spoofed or leftover Flow Control frame is received concurrently on
another CPU:

net/can/isotp.c:isotp_rcv_fc() {
    ...
    if (so->tx.state != ISOTP_WAIT_FC &&
        so->tx.state != ISOTP_WAIT_FIRST_FC)
    ...
}

The softirq path can see ISOTP_WAIT_FIRST_FC and immediately transition to
ISOTP_SENDING, transmitting a Consecutive Frame before isotp_sendmsg() has
sent the First Frame, scrambling the protocol state.

[Severity: Critical]
This is a pre-existing issue, but can the signal error path in
isotp_sendmsg() leak uninitialized kernel memory?

If isotp_sendmsg() is interrupted by a signal, it sets the state to idle:

net/can/isotp.c:isotp_sendmsg() {
    ...
    so->tx.state = ISOTP_IDLE;
    hrtimer_cancel(&so->txfrtimer);
    ...
}

If a concurrent isotp_rcv_fc() already passed the state check, and
userspace immediately calls sendmsg() again with a large size:

net/can/isotp.c:isotp_sendmsg() {
    ...
    u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
    ...
}

The delayed isotp_rcv_fc() could resume, transition the state, and transmit
the newly allocated, uninitialized so->tx.buf over the CAN bus before the
data has been copied from userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=1