Re: [PATCH v9 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: isotp: incomplete rollback and premature rx registration in isotp_bind()
--

commit b2feba7ed89eac12b701d3372fa7b7613b9b4096
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: check the return values of can_rx_register() in isotp_bind()

This commit checks the return values of can_rx_register() in isotp_bind()
for the RX and echo receivers. If registering the echo receiver fails, it
unregisters the previously registered RX receiver and returns the error.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 1a272ce6a2bde..da80ef606ce09 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1624,16 +1624,25 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  
>  	ifindex = dev->ifindex;
>  
> -	if (isotp_register_rxid(so))
> -		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> -				isotp_rcv, sk, sock_i_ino(sk), sk);
> +	if (isotp_register_rxid(so)) {
> +		err = can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> +				      isotp_rcv, sk, sock_i_ino(sk), sk);
> +		if (err)
> +			goto out_put_dev;
> +	}
>  
>  	/* no consecutive frame echo skb in flight */
>  	WRITE_ONCE(so->cfecho, 0);
>  
>  	/* register for echo skb's */
> -	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> -			isotp_rcv_echo, sk, sock_i_ino(sk), sk);
> +	err = can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> +			      isotp_rcv_echo, sk, sock_i_ino(sk), sk);
> +	if (err) {
> +		if (isotp_register_rxid(so))
> +			can_rx_unregister(net, dev, rx_id, SINGLE_MASK(rx_id),
> +					  isotp_rcv, sk);
> +		goto out_put_dev;
> +	}

[Severity: Medium]
Can this rollback leave the socket permanently wedged or with stale state if
a frame arrives while the second can_rx_register() is running?

In net/can/isotp.c:isotp_bind(), the first can_rx_register() publishes
isotp_rcv to the device's RCU receive list while so->ifindex, so->rxid,
so->txid, and so->bound are still uninitialized (or zero), and then the
second can_rx_register() allocates with GFP_KERNEL. Because isotp_rcv() runs
in softirq without checking so->bound, incoming frames matching rx_id can be
processed during this window:

1. If a First Frame arrives, isotp_rcv() -> isotp_rcv_ff() sets
so->rx.state to ISOTP_WAIT_DATA:

net/can/isotp.c:isotp_rcv_ff() {
	...
	/* initial setup for this pdu reception */
	so->rx.sn = 1;
	WRITE_ONCE(so->rx.state, ISOTP_WAIT_DATA);

	/* no creation of flow control frames */
	if (so->opt.flags & CAN_ISOTP_LISTEN_MODE)
		return 0;

	/* send our first FC frame */
	isotp_send_fc(sk, ae, ISOTP_FC_CTS);
	return 0;
}

When CAN_ISOTP_LISTEN_MODE is not set, isotp_send_fc() looks up so->ifindex
(which is still 0) and returns before starting so->rxtimer:

net/can/isotp.c:isotp_send_fc() {
	...
	dev = dev_get_by_index(sock_net(sk), so->ifindex);
	if (!dev) {
		kfree_skb(nskb);
		return 1;
	}
	...
	hrtimer_start(&so->rxtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT);
	return 0;
}

If the second can_rx_register() then fails with -ENOMEM, can_rx_unregister()
removes isotp_rcv without resetting so->rx.state to ISOTP_IDLE. With
isotp_rcv unregistered and so->rxtimer never armed, so->rx.state stays in
ISOTP_WAIT_DATA forever, causing all future isotp_bind() attempts on the
socket to fail with -EAGAIN:

net/can/isotp.c:isotp_bind() {
	...
	if (READ_ONCE(so->tx.state) != ISOTP_IDLE ||
	    READ_ONCE(so->rx.state) != ISOTP_IDLE) {
		err = -EAGAIN;
		goto out;
	}
	...
}

2. If a Single Frame arrives in this window before the second
can_rx_register() fails, isotp_rcv() -> isotp_rcv_sf() queues an skb onto
sk->sk_receive_queue of the unbound socket, which cannot be read while
unbound and would be delivered as stale data if the socket is later bound.

3. Because can_rx_unregister() only schedules deletion via call_rcu(), an
in-flight isotp_rcv() callback can still be running after isotp_bind() drops
lock_sock(sk) and returns an error, racing with subsequent isotp_setsockopt()
calls that modify so->opt, so->rxfc, and so->ll without taking so->rx_lock
when so->bound is 0.

Would it make sense to initialize the socket fields before registering the
receivers (or guard isotp_rcv() on so->bound) and ensure any in-flight
receiver and partial RX state are cleaned up on failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025-can-rx-lock-v9-0-7ef2a2b5c2b4@hartkopp.net?part=5