Re: [PATCH v8 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

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

Pre-existing issues:
- [Critical] A TOCTOU race condition in `isotp_sendmsg` allows concurrent modification of socket options, leading to an SKB heap overflow and `skb_over_panic`.
- [High] The check in `isotp_bind()` to prevent rebinding during an active state machine does not fully prevent cross-interface packet injection from RCU-delayed softirqs.
--

--- Patch [2]: [PATCH v8 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER ---
commit 508c0cfe152e6936d33176beb93ef03692ea4ea7
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

This tracks the bound net_device with a reference instead of re-resolving
it from ifindex during isotp_release(). It also rejects isotp_bind() with
-EAGAIN if the state machines aren't idle yet, avoiding timers acting on a
newly bound interface.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index d30937345bcdc..44c044eb83e12 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -978,6 +980,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			goto err_event_drop;
>  	}
>  
> +	/* so->bound is only checked once above - a wakeup may have
> +	 * unbound/rebound the socket meanwhile, so re-validate it
> +	 */
> +	if (!so->bound) {
> +		err = -EADDRNOTAVAIL;
> +		goto err_out_drop;
> +	}

[Severity: Critical]
This is a pre-existing issue, but does a time-of-check to time-of-use race
still exist here if sock_alloc_send_skb() blocks later in isotp_sendmsg()?

If sock_alloc_send_skb(sk, so->ll.mtu) blocks during allocation, a concurrent
NETDEV_UNREGISTER could trigger isotp_notify(), which acquires lock_sock()
and sets so->bound = 0.

Userspace could then call setsockopt(CAN_ISOTP_LL_OPTS), which passes its
!so->bound check and modifies so->ll.mtu and so->tx.ll_dl.

When isotp_sendmsg() resumes, it has a smaller SKB but could use the new
larger MTU, or isotp_fill_dataframe() could use the new so->tx.ll_dl to copy
too many bytes into the small SKB:

net/can/isotp.c:isotp_fill_dataframe() {
    ...
    for (i = 0; i < num; i++)
        cf->data[pcilen + i] = so->tx.buf[so->tx.idx++];
    ...
}

Could this lead to a heap out-of-bounds write overwriting skb_shared_info?

[ ... ]

> @@ -1317,6 +1327,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> +	/* A transmission or reception that outlived a previous binding
> +	 * (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo
> +	 * and RX watchdog timers bound how long this takes. Checked together
> +	 * with so->bound in the same lock_sock() section above, so there is
> +	 * no window in which a concurrent isotp_notify() could be missed.
> +	 */
> +	if (so->tx.state != ISOTP_IDLE || so->rx.state != ISOTP_IDLE) {
> +		err = -EAGAIN;
> +		goto out;
> +	}

[Severity: High]
This is a pre-existing issue, but does this state machine check fully prevent
cross-interface packet injection from RCU-delayed softirqs?

If a packet arrives on an old device and is queued via can_rx_register()'s
softirq, and concurrently the device is unregistered (clearing so->bound),
userspace could call bind() for a new device.

Since isotp_rcv() hasn't acquired so->rx_lock to change the state yet, the
lockless check here passes and so->dev is updated. Could the delayed softirq
then acquire so->rx_lock, transition the state, and send a Flow Control frame
using the newly bound so->ifindex?

Would isotp_rcv() need to verify that skb->dev matches the currently bound
so->dev after acquiring so->rx_lock?

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