Re: [PATCH v14 03/11] 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:

New issues:
- [Medium] Split critical sections in `bcm_rx_setup()` create a window of inconsistent state.

Pre-existing issues:
- [High] Lockless reads and writes of timer values and flags in TX path cause a data race and potential torn reads.
- [High] Direct, lockless modification of `op->frames` array in `bcm_tx_setup()` leads to data races and unrecoverable state corruption.
- [High] Use of `list_add()` instead of `list_add_rcu()` causes data races with lockless RCU readers.
--

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

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

This commit introduces bcm_rx_update_lock to protect timer and filter data
updates in bcm_rx_setup() and various RX handlers. It also adds read-side
locking in bcm_can_tx() and normalizes RTR frames in a staging buffer
before registering them.

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 this read-side lock actually protect
against concurrent updates?

While bcm_can_tx() acquires bcm_tx_lock here, the writer in bcm_tx_setup()
directly modifies the op->frames array without holding the lock and without
using a staging buffer:

bcm_tx_setup() {
    ...
    for (i = 0; i < msg_head->nframes; i++) {
        cf = op->frames + op->cfsiz * i;
        err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Because memcpy_from_msg() can sleep, it cannot be called under a spinlock.
Could this lead to bcm_can_tx() copying partially overwritten CAN frames, or
unrecoverable state corruption if memcpy_from_msg() fails midway?

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but can this result in torn reads on 32-bit
platforms?

The bcm_tx_timeout_handler() reads kt_ival1, count, and flags locklessly
here. Concurrently, a user calling sendmsg() with TX_SETUP can trigger
bcm_tx_setup() to write new timer values locklessly:

bcm_tx_setup() {
    ...
    if (op->flags & SETTIMER) {
        op->ival1 = msg_head->ival1;
        op->ival2 = msg_head->ival2;
        op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
        op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    }
    ...
}

Does this data race risk unpredictable timer expiration or spurious CAN
transmissions if inconsistent combinations of flags and counts are observed?

[ ... ]

> @@ -1175,19 +1230,40 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  			return -E2BIG;
>  
>  		if (msg_head->nframes) {
> -			/* update CAN frames content */
> -			err = memcpy_from_msg(op->frames, msg,
> +			/* get new CAN frames content before locking */
> +			new_frames = kmalloc(msg_head->nframes * op->cfsiz,
> +					     GFP_KERNEL);
> +			if (!new_frames)
> +				return -ENOMEM;
> +
> +			err = memcpy_from_msg(new_frames, msg,
>  					      msg_head->nframes * op->cfsiz);
> -			if (err < 0)
> +			if (err < 0) {
> +				kfree(new_frames);
>  				return err;
> +			}
>  
> -			/* clear last_frames to indicate 'nothing received' */
> -			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
> +			bcm_rx_setup_rtr_check(msg_head, op, new_frames);
>  		}
>  
> +		spin_lock_bh(&op->bcm_rx_update_lock);
>  		op->nframes = msg_head->nframes;
>  		op->flags = msg_head->flags;
>  
> +		if (msg_head->nframes) {
> +			/* update CAN frames content */
> +			memcpy(op->frames, new_frames,
> +			       msg_head->nframes * op->cfsiz);
> +
> +			/* clear last_frames to indicate 'nothing received' */
> +			memset(op->last_frames, 0,
> +			       msg_head->nframes * op->cfsiz);
> +		}
> +		spin_unlock_bh(&op->bcm_rx_update_lock);

[ ... ]

> @@ -1266,29 +1345,20 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	/* check flags */
>  
>  	if (op->flags & RX_RTR_FRAME) {
> -		struct canfd_frame *frame0 = op->frames;
> -
>  		/* no timers in RTR-mode */
>  		hrtimer_cancel(&op->thrtimer);
>  		hrtimer_cancel(&op->timer);
> -
> -		/*
> -		 * 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 ... ;-]
> -		 */
> -		if ((op->flags & TX_CP_CAN_ID) ||
> -		    (frame0->can_id == op->can_id))
> -			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> -
>  	} else {
>  		if (op->flags & SETTIMER) {
>  
>  			/* set timer value */
> +			spin_lock_bh(&op->bcm_rx_update_lock);
>  			op->ival1 = msg_head->ival1;
>  			op->ival2 = msg_head->ival2;

[Severity: Medium]
Could a concurrent bcm_rx_handler() process a frame during the gap between
these two critical sections?

In bcm_rx_setup(), the lock is acquired to update flags and frames, then
dropped, and re-acquired here to update the timer values. Does this create
a window where bcm_rx_handler() might see the new flags and frames alongside
stale kt_ival1 and kt_ival2 timer values, resulting in incorrect throttling
behavior?

[Severity: High]
This is a pre-existing issue, but does publishing operations without RCU
primitives cause data races with lockless readers?

In bcm_tx_setup() and bcm_rx_setup(), operations are published to the lists
using list_add() rather than list_add_rcu():

bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
    ...
}

However, readers like bcm_proc_show() traverse the list locklessly:

bcm_proc_show() {
    ...
    list_for_each_entry(op, &bo->tx_ops, list) {
    ...
}

Since list_add() lacks the smp_store_release() barrier provided by
list_add_rcu(), can lockless readers observe uninitialized memory if they
reach a newly linked node before its contents are globally visible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v14-0-867a4be60a61@hartkopp.net?part=3