Re: [PATCH v10 3/3] can: isotp: serialize TX state transitions under so->rx_lock

On 12.07.26 20:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Lockless state transition in the timeout handler creates a race condition that causes timeout errors to be silently dropped or misattributed, leading sendmsg() to erroneously return success for timed-out transfers.

The patch set fixes potential UAF and stalls.

The error return codes especially for (uncommon) multi-threaded users 
have been there for years and might be fixed in later patches.

So no reason to delay this patch set.

Best regards,
Oliver

> --
> 
> --- Patch [3]: [PATCH v10 3/3] can: isotp: serialize TX state transitions under so->rx_lock ---
> commit e21ce85bab56b1b57ffd11ce2b23b760bfb72c21
> Author: Oliver Hartkopp <[email protected]>
> 
> can: isotp: serialize TX state transitions under so->rx_lock
> 
> This commit serializes CAN ISOTP TX state transitions and timer cancellations
> under so->rx_lock to prevent concurrent modifications. It also changes the
> timeout handlers to locklessly cancel transfers by checking and updating
> so->tx_gen and so->tx.state.
> 
> Link: https://lore.kernel.org/linux-can/[email protected]/
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 44c044eb83e12..54becaf6898f1 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
> [ ... ]
>> @@ -893,53 +927,83 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
> [ ... ]
>> +static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>>   {
>> -	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
>> -					     txtimer);
>>   	struct sock *sk = &so->sk;
>> +	u32 gen = READ_ONCE(so->tx_gen);
>> +	u32 old_state = READ_ONCE(so->tx.state);
>>   
>>   	/* don't handle timeouts in IDLE or SHUTDOWN state */
>> -	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
>> +	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
>> +		return HRTIMER_NORESTART;
>> +
>> +	/* only claim the timeout if the state is still unchanged */
>> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
>>   		return HRTIMER_NORESTART;
>>   
>>   	/* we did not get any flow control or echo frame in time */
>>   
>> -	/* report 'communication error on send' */
>> -	sk->sk_err = ECOMM;
>> -	if (!sock_flag(sk, SOCK_DEAD))
>> -		sk_error_report(sk);
>> +	if (READ_ONCE(so->tx_gen) == gen) {
>> +		/* report 'communication error on send' */
>> +		sk->sk_err = ECOMM;
>> +		if (!sock_flag(sk, SOCK_DEAD))
>> +			sk_error_report(sk);
>> +	}
> 
> [Severity: High]
> Does this lockless state transition create a race window where timeout
> errors are lost?
> 
> By setting so->tx.state to ISOTP_IDLE before setting sk->sk_err = ECOMM,
> a thread waiting in isotp_sendmsg() for wait_tx_done might wake up, observe
> ISOTP_IDLE, and read sock_error() before the error is set, falsely returning
> success.
> 
> Additionally, could another concurrent sendmsg() see the IDLE state,
> claim the socket, and increment tx_gen before the error is set?
> 
> If this happens, the check if (READ_ONCE(so->tx_gen) == gen) will fail,
> and the error will never be reported to the original waiting thread.
> 
> [ ... ]
>> @@ -960,13 +1024,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> [ ... ]
>> +	/* new transfer: bump so->tx_gen and drain the old one's timers,
>> +	 * still under the so->rx_lock we just claimed the socket with
>> +	 */
>> +	WRITE_ONCE(so->tx.state, ISOTP_SENDING);
>> +	WRITE_ONCE(so->tx_gen, READ_ONCE(so->tx_gen) + 1);
>> +	hrtimer_cancel(&so->txtimer);
>> +	hrtimer_cancel(&so->echotimer);
> 
> [Severity: High]
> When this tx_gen increment occurs concurrently with isotp_tx_timeout()
> after the timeout has already transitioned the state to ISOTP_IDLE, doesn't it
> cause the timeout handler to suppress the error for the previous transfer?
> 
> [ ... ]
>