Re: [PATCH v4 7/7] can: bcm: validate frame length in bcm_rx_setup() for RTR replies

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race on `op->rx_ifindex` and `op->rx_stamp` in `bcm_rx_handler` leading to incorrect metadata being reported to userspace.
--

--- Patch [7]: [PATCH v4 7/7] can: bcm: validate frame length in bcm_rx_setup() for RTR replies ---
commit 1023235e0f18b95b00bedbafd97f5943d8506566
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: validate frame length in bcm_rx_setup() for RTR replies

This commit fixes a missing frame length check in bcm_rx_setup() for
RTR-reply frames configured via RX_SETUP with RX_RTR_FRAME. It ensures
cf->len is validated against CAN and CAN FD limits before installation.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index f04216723e0fc..d4d2c89dc4009 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1467,6 +1482,14 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	}
>  
>  	return msg_head->nframes * op->cfsiz + MHSIZ;
> +
> +free_op:
> +	if (op->frames != &op->sframe)
> +		kfree(op->frames);
> +	if (op->last_frames != &op->last_sframe)
> +		kfree(op->last_frames);
> +	kfree(op);
> +	return err;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race in
bcm_rx_handler() involving op->rx_ifindex and op->rx_stamp.

When a user creates a BCM socket listening on all interfaces (ifindex=0),
and CAN frames matching the filter arrive concurrently on multiple
interfaces (e.g., handled by different CPUs in softirq), bcm_rx_handler()
can execute concurrently. Both CPUs appear to write to op->rx_stamp and
op->rx_ifindex without synchronization before acquiring 
op->bcm_rx_update_lock:

net/can/bcm.c:bcm_rx_handler() {
	...
	op->rx_stamp = skb->tstamp;
	/* save originator for recvfrom() */
	op->rx_ifindex = skb->dev->ifindex;
	...
}

Later, bcm_send_to_user() reads these fields to send metadata to userspace:

net/can/bcm.c:bcm_send_to_user() {
	...
	skb->tstamp = op->rx_stamp;
	...
	addr = (struct sockaddr_can *)skb->cb;
	memset(addr, 0, sizeof(*addr));
	addr->can_family  = AF_CAN;
	addr->can_ifindex = op->rx_ifindex;
	...
}

Could this lead to torn reads of the 64-bit rx_stamp on 32-bit platforms
and incorrect rx_ifindex values being reported to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v4-0-aa87b8b9d33f@hartkopp.net?part=7