Re: [PATCH v4 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected modification of RX operation buffers from process context races with softirq receive handlers.
--

--- Patch [2]: [PATCH v4 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 9ced6bfc5d2fbc4191944246b0050dabfa00318e
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

This commit fixes a race condition between bcm_sendmsg() and socket state
modifications by moving the ifindex read inside the socket lock. It also
fixes a silent failure when a target device disappears concurrently during
RX_SETUP.

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;
>  			}

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying
bcm_rx_setup(), I noticed a potential race condition. Does the process
context modification of RX buffers race with the softirq receive handlers?

When a user application repeatedly updates a BCM RX_SETUP filter via
sendmsg(), it modifies op->frames and resets op->last_frames directly in
process context:

net/can/bcm.c:bcm_rx_setup() {
    ...
			err = memcpy_from_msg(op->frames, msg,
					      msg_head->nframes * op->cfsiz);
			if (err < 0)
				return err;

			/* clear last_frames to indicate 'nothing received' */
			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
    ...
}

Concurrently, if a matching CAN frame arrives, will bcm_rx_handler() in
softirq context locklessly read op->frames and update op->last_frames?

net/can/bcm.c:bcm_rx_handler() {
    ...
		for (i = 1; i < op->nframes; i++) {
			if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==
			    (get_u64(op->frames, 0) &
			     get_u64(op->frames + op->cfsiz * i, 0))) {
				bcm_rx_cmp_to_index(op, i, rxframe,
						    traffic_flags);
				break;
    ...
}

Could this lead to torn reads or writes if the filter logic misfires while
these buffers are observed in an inconsistent state, potentially delivering
corrupted or duplicate CAN frames to userspace?

[ ... ]

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