Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal

On 14.07.26 12:44, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.

Fixed in patch 10

> --
> 
> --- Patch [9]: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal ---
> commit 7ac0666664aa8f0bbda8367494a6678c948d2082
> Author: Oliver Hartkopp <[email protected]>
> 
> can: bcm: fix stale rx/tx ops after device removal
> 
> This commit fixes issues where an RX_SETUP update could silently skip
> registering if a concurrent NETDEV_UNREGISTER occurred. It also ensures
> that cyclic transmissions are stopped when a device is removed.
> 
> Link: https://lore.kernel.org/linux-can/[email protected]/
> Link: https://lore.kernel.org/linux-can/[email protected]/
> 
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 5660165cdf0eb..10dd99ecce70e 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
> [ ... ]
>> @@ -1483,18 +1495,32 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>   				err = -ENODEV;
>>   			}
>>   
>> -		} else
>> +		} else {
>>   			err = can_rx_register(sock_net(sk), NULL, op->can_id,
>>   					      REGMASK(op->can_id),
>>   					      bcm_rx_handler, op, "bcm", sk);
>> +		}
> 
> [Severity: High]
> This is a pre-existing issue, but is there a data race when bcm_rx_handler()
> accesses op->rx_stamp and op->rx_ifindex?
> 
> When a BCM socket is bound to ifindex 0 (all CAN devices), multiple CAN
> frames arriving simultaneously on different CAN interfaces trigger
> bcm_rx_handler() concurrently. The handler unconditionally writes to
> op->rx_stamp and op->rx_ifindex outside of op->bcm_rx_update_lock:
> 
> net/can/bcm.c:bcm_rx_handler() {
>      ...
> 	/* save rx timestamp */
> 	op->rx_stamp = skb->tstamp;
> 	/* save originator for recvfrom() */
> 	op->rx_ifindex = skb->dev->ifindex;
>      ...
> }
> 
> These fields are then read in bcm_send_to_user(), sometimes locklessly (e.g.
> when called from bcm_rx_timeout_handler()):
> 
> net/can/bcm.c:bcm_send_to_user() {
>      ...
> 		/* restore rx timestamp */
> 		skb->tstamp = op->rx_stamp;
>      ...
> 	addr->can_ifindex = op->rx_ifindex;
>      ...
> }
> 
> Does this cause torn 64-bit reads of op->rx_stamp on 32-bit architectures,
> and can the interface index be logically swapped with that of a concurrent
> frame?
> 
> [ ... ]
>