Re: [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Ignored CAN frames incorrectly overwrite the `rx_stamp` and `rx_ifindex` of pending throttled frames, leading to incorrect metadata delivery to userspace.
--

--- Patch [9]: [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() ---
commit 1f01914b41bcf86b5db9980430a19a39e9075e09
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()

This commit fixes a data race on op->rx_stamp and op->rx_ifindex by moving
their assignment inside the bcm_rx_update_lock critical section. It also
stops updating them for RTR-request frames.

Link: https://lore.kernel.org/linux-can/20260707145135.5BC831F00A3A@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 4bdca72f9327f..73e3a5fc4342f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -805,6 +800,14 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  
>  	spin_lock_bh(&op->bcm_rx_update_lock);
>  
> +	/* save rx timestamp and originator for recvfrom() under lock.
> +	 * For an op subscribed on all interfaces (ifindex == 0)
> +	 * bcm_rx_handler() can run concurrently on different CPUs so
> +	 * the CAN content and the meta data must be bundled correctly.
> +	 */
> +	op->rx_stamp = skb->tstamp;
> +	op->rx_ifindex = skb->dev->ifindex;

[Severity: High]
This is a pre-existing issue, but does unconditionally overwriting these
fields corrupt the metadata for pending throttled frames?

If a user configures a BCM socket with a filter (e.g., multiplex nframes > 1),
bound to all interfaces (ifindex == 0), with a throttle timer:

1. A frame arrives on vcan0, matches a mask, and is throttled.
   op->rx_ifindex becomes vcan0.
2. A subsequent frame arrives on vcan1 but does not match any multiplex mask.

Since bcm_rx_handler() unconditionally updates op->rx_ifindex here before
applying the filters, doesn't it overwrite the metadata with vcan1 even
though the frame is ignored?

When the throttle timer expires, bcm_send_to_user() will use this corrupted
op->rx_ifindex to deliver the throttled frame from vcan0, making it appear as
if it came from vcan1.

Also, since the bcm_op struct only has one rx_stamp and rx_ifindex field,
doesn't this mean it structurally lacks the capacity to store independent
metadata per multiplex mask if multiple multiplexed frames are throttled
simultaneously?

> +
>  	if (op->flags & RX_FILTER_ID) {
>  		/* the easiest case */
>  		bcm_rx_update_and_send(op, op->last_frames, rxframe,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=9