Re: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

On 14.07.26 12:33, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Unsafe lockless iteration of `bo->tx_ops` and `bo->rx_ops` in procfs, using `list_add()` instead of `list_add_rcu()` for list updates and `list_for_each_entry()` instead of `list_for_each_entry_rcu()`.
> - [High] `bcm_rx_setup()` modifies an active `rx_op`'s data and state from process context without any synchronization with `bcm_rx_handler()` running in softirq context.
> - [High] `bcm_tx_setup()` modifies an active `tx_op`'s frames and flags without holding `bcm_tx_lock`, racing with the softirq timer and transmission paths.
> --
> 

Fixed later.

> --- Patch [2]: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
> commit 79233a73945b83eac56e8c17536d2f019c104f50
> Author: Oliver Hartkopp <[email protected]>
> 
>      can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
>      
>      bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
>      lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
>      mutate both fields under that same lock. Because the lockless reads
>      and the locked writes are unordered with respect to each other, a
>      racing bcm_notify() (device unregister) or bcm_connect() (concurrent
>      bind on another thread sharing the socket) can make bcm_sendmsg()
>      observe an inconsistent combination, e.g. a stale bound=1 together
>      with the now-cleared ifindex=0, silently turning a socket bound to a
>      specific CAN interface into one that also matches "any" interface.
> 
>      Link: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
> 
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index bdf53241bd7b3..b612135b017d1 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>   
>>   				op->rx_reg_dev = dev;
>>   				dev_put(dev);
>> +			} else {
>> +				/* the requested device is gone - do not
>> +				 * silently succeed without registering
>> +				 */
>> +				err = -ENODEV;
>>   			}
>>   
>>   		} else
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does bcm_rx_setup() safely
> modify an active rx_op's data and state from process context without
> synchronization with bcm_rx_handler() running in softirq context?
> 
> bcm_rx_setup() directly overwrites op->frames via memcpy_from_msg() and
> clears op->last_frames while only holding lock_sock(sk):
> 
> net/can/bcm.c:bcm_rx_setup() {
>      ...
>      err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
>      ...
> }
> 
> Concurrently, bcm_rx_handler() executes locklessly in the softirq RX path
> and reads these frames:
> 
> net/can/bcm.c:bcm_rx_handler() {
>      ...
>      if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) == ...
>      ...
> }
> 
> Since memcpy_from_msg() copies from userspace and can block or be preempted,
> could the softirq observe partially updated frames, mismatched lengths, or
> torn flags, leading to incorrect CAN message filtering and potential data
> corruption?
> 
> [ ... ]
>> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>>   			struct net_device *dev;
>>   
>>   			dev = dev_get_by_index(sock_net(sk), ifindex);
>> -			if (!dev)
>> -				return -ENODEV;
>> +			if (!dev) {
>> +				ret = -ENODEV;
>> +				goto out_release;
>> +			}
>>   
>>   			if (dev->type != ARPHRD_CAN) {
>>   				dev_put(dev);
>> -				return -ENODEV;
>> +				ret = -ENODEV;
>> +				goto out_release;
>>   			}
>>   
>>   			dev_put(dev);
>>   		}
>>   	}
>>   
>> -	lock_sock(sk);
>> -
>>   	switch (msg_head.opcode) {
>>   
>>   	case TX_SETUP:
> 
> [Severity: High]
> This is a pre-existing issue, but does bcm_tx_setup() modify an active
> tx_op's frames and flags without holding bcm_tx_lock, racing with the softirq
> timer and transmission paths?
> 
> When updating an existing tx op, bcm_tx_setup() calls memcpy_from_msg() to
> update op->frames before acquiring bcm_tx_lock:
> 
> net/can/bcm.c:bcm_tx_setup() {
>      ...
>      err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
>      ...
> }
> 
> Meanwhile, bcm_can_tx() drops the lock and calls skb_put_data() to copy the
> frame into an skb:
> 
> net/can/bcm.c:bcm_can_tx() {
>      ...
>      skb_put_data(skb, cf, op->cfsiz);
>      ...
> }
> 
> Could this data race allow the softirq to transmit partially updated CAN
> frames onto the bus? Because memcpy_from_msg() accesses userspace and might
> block, the race window appears wide enough to result in deterministic
> corruption of transmitted frames under concurrent updates.
> 
> [Severity: High]
> This is also a pre-existing issue, but is it safe for bcm_proc_show() to
> iterate over bo->tx_ops and bo->rx_ops locklessly in procfs, while
> bcm_tx_setup() and bcm_rx_setup() use list_add() instead of list_add_rcu()?
> 
> bcm_tx_setup() and bcm_rx_setup() insert new operations using the plain
> list_add() macro, which lacks the smp_store_release() memory barrier:
> 
> net/can/bcm.c:bcm_tx_setup() {
>      ...
>      list_add(&op->list, &bo->tx_ops);
>      ...
> }
> 
> bcm_proc_show() traverses tx_ops locklessly under rcu_read_lock(), but uses
> the unsafe list_for_each_entry() macro entirely bypassing RCU read primitives:
> 
> net/can/bcm.c:bcm_proc_show() {
>      ...
>      list_for_each_entry(op, &bo->tx_ops, list) {
>      ...
> }
> 
> Could a concurrent procfs reader observe the list pointer before the bcm_op
> structure is fully visible in memory, causing a dereference of uninitialized
> memory or a crash?
> 
>>   		ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
>>   		break;
>