Re: [PATCH v10 03/10] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless `memcpy_from_msg()` in `bcm_tx_setup()` defeats the `bcm_can_tx()` lock-protected snapshot, leading to torn CAN frames and potentially transmitting unvalidated malicious lengths.
- [High] Missing frame payload length validation for `RX_RTR_FRAME` operations in `bcm_rx_setup()`, leading to downstream out-of-bounds reads during automated RTR replies.
- [Medium] Metadata `rx_stamp`, `rx_ifindex`, and `frames_abs` are updated outside the newly introduced `bcm_rx_update_lock` in `bcm_rx_handler()`, leading to torn metadata for concurrent packet receptions.
- [Medium] Timer fields and counters (`kt_ival1`, `kt_ival2`, `count`) are read without locking in `bcm_tx_timeout_handler()`, leading to potential logic flaws and torn reads.
--

--- Patch [3]: [PATCH v10 03/10] can: bcm: add locking when updating filter and timer values ---
commit 3f8b487ac0338cf76d904d74f58ed7c66c12d1fb
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

This patch adds a new per-op bcm_rx_update_lock to protect timer and filter
updates in bcm_rx_setup() against concurrent access from RX handlers. It
also stages memcpy_from_msg() into a temporary buffer to avoid sleeping
while holding a spinlock, and closes a race condition where a stale reply
with CAN_RTR_FLAG could be transmitted.

Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..af4bb4182bc29 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -293,22 +294,26 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>   * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>   *              of the given bcm tx op
>   */
> -static void bcm_can_tx(struct bcm_op *op)
> +static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
>  {
>  	struct sk_buff *skb;
>  	struct can_skb_ext *csx;
>  	struct net_device *dev;
> -	struct canfd_frame *cf;
> +	struct canfd_frame cframe;
>  	int err;
>  
>  	/* no target device? => exit */
>  	if (!op->ifindex)
>  		return;
>  
> -	/* read currframe under lock protection */
> -	spin_lock_bh(&op->bcm_tx_lock);
> -	cf = op->frames + op->cfsiz * op->currframe;
> -	spin_unlock_bh(&op->bcm_tx_lock);
> +	if (!cf) {
> +		/* read currframe under lock protection */
> +		spin_lock_bh(&op->bcm_tx_lock);
> +		memcpy(&cframe, op->frames + op->cfsiz * op->currframe,
> +		       op->cfsiz);
> +		cf = &cframe;
> +		spin_unlock_bh(&op->bcm_tx_lock);
> +	}

[Severity: High]
This is a pre-existing issue, but does bcm_tx_setup() update op->frames
locklessly?

If a concurrent bcm_can_tx() is driven by the TX timer while the user
updates an existing TX operation, bcm_tx_setup() copies data into the active
op->frames without taking a lock:

net/can/bcm.c:bcm_tx_setup() {
    ...
    cf = op->frames + op->cfsiz * i;
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Because memcpy_from_msg() can sleep, it happens without the spinlock,
which could allow an attacker to concurrently write a malicious payload
length.

Could this lead to a torn read of the CAN frame in bcm_can_tx() and a
transmission of an unvalidated length, resulting in an out-of-bounds read?

[ ... ]

> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  	struct bcm_msg_head msg_head;
>  
>  	if (op->kt_ival1 && (op->count > 0)) {
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {

[Severity: Medium]
This is a pre-existing issue, but are the timer and count fields read here
subject to concurrent updates?

bcm_tx_timeout_handler() evaluates op->kt_ival1, op->kt_ival2, and op->count
without holding the bcm_tx_lock.

Meanwhile, bcm_tx_setup() can update these timer values locklessly in process
context. Additionally, op->count can be decremented by another thread executing
bcm_can_tx() concurrently.

Could this lead to torn reads of 64-bit ktime_t values on 32-bit platforms,
causing unexpected timer expiries or incorrect notifications?

[ ... ]

> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;
>  
> -	if (op->flags & RX_RTR_FRAME) {
> +	/* snapshot the flag under lock: op->flags/op->frames may be updated
> +	 * concurrently by bcm_rx_setup().
> +	 */
> +	spin_lock_bh(&op->bcm_rx_update_lock);
> +	rtr_frame = op->flags & RX_RTR_FRAME;
> +	if (rtr_frame)
> +		memcpy(&rtrframe, op->frames, op->cfsiz);
> +	spin_unlock_bh(&op->bcm_rx_update_lock);

[Severity: Medium]
This is a pre-existing issue, but are some metadata updates still left
unprotected by the new lock?

Just above this section, metadata fields are modified outside the lock:

net/can/bcm.c:bcm_rx_handler() {
    ...
    op->rx_stamp = skb->tstamp;
    op->rx_ifindex = skb->dev->ifindex;
    ...
    op->frames_abs++;
    ...
}

If an operation is bound to all interfaces (ifindex = 0) and receives frames
concurrently on multiple CPUs, could these updates overwrite op->rx_ifindex
before bcm_send_to_user() dispatches it inside the critical section?

Would this result in userspace receiving incorrectly bundled metadata,
such as a packet from can0 tagged with the timestamp and ifindex of can1?

[ ... ]

> @@ -1130,6 +1165,24 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	return err;
>  }
>  
> +static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
> +				   struct bcm_op *op, void *new_frames)
> +{
> +	/* funny feature in RX(!)_SETUP only for RTR-mode:
> +	 * copy can_id into frame BUT without RTR-flag to
> +	 * prevent a full-load-loopback-test ... ;-]
> +	 * normalize this on the staged buffer, before it is
> +	 * ever installed into op->frames.
> +	 */
> +	if (msg_head->flags & RX_RTR_FRAME) {
> +		struct canfd_frame *frame0 = new_frames;
> +
> +		if ((msg_head->flags & TX_CP_CAN_ID) ||
> +		    frame0->can_id == op->can_id)
> +			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> +	}
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does the handling for
RX_RTR_FRAME lack payload length validation?

If a user configures an RX_SETUP operation with RX_RTR_FRAME and provides
a CAN frame with a malicious length (for example, len = 255),
bcm_rx_setup_rtr_check() normalizes the RTR flag but doesn't validate
the payload length bounds.

When an incoming RTR frame triggers the automated reply, bcm_rx_handler()
passes this unvalidated frame to bcm_can_tx():

net/can/bcm.c:bcm_rx_handler() {
    ...
    if (rtr_frame) {
        bcm_can_tx(op, &rtrframe);
        return;
    }
    ...
}

Could the CAN driver trust cf->len and read beyond the allocated buffer,
leading to an out-of-bounds slab read that leaks kernel memory onto the bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v10-0-57fff38e34ce@hartkopp.net?part=3