Re: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent timer updates lead to hrtimer_forward_now being called with a zero interval, triggering a WARN_ON.
- [High] Missing locking in the TX path leads to data races and torn reads for CAN frames and timer values.
--

--- Patch [3]: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values ---
commit 29dee20a547c38c99b2b856375f9e172e6a33181
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

    KCSAN detected a simultaneous access to timer values that can be
    overwritten in bcm_rx_setup() when updating timer and filter content
    while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
    run concurrently on incoming CAN traffic.

    Closes: 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..fd548c6eaa913 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 isn't a bug introduced by this patch, but does bcm_tx_lock actually
prevent concurrent overwrites by bcm_tx_setup()?

The commit message claims to "take a lock-protected snapshot of the currframe
in bcm_can_tx() to avoid partly overwrites by content updates in
bcm_tx_setup()".

However, looking at bcm_tx_setup(), it appears to copy user data directly into
op->frames without holding bcm_tx_lock:

net/can/bcm.c: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);
    ...
}

If the writer never acquires the lock (since memcpy_from_msg() can sleep),
doesn't this lock in bcm_can_tx() remain ineffective against races?

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but is this lockless read of kt_ival1 vulnerable
to torn reads on 32-bit systems?

bcm_tx_setup() appears to update kt_ival1 and kt_ival2 locklessly, and this
handler reads them locklessly. Could this cause the timer to fire at
unintended intervals if a 64-bit ktime_t is partially updated while being read?

> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {

[ ... ]

> @@ -686,15 +695,22 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
>  static enum hrtimer_restart bcm_rx_thr_handler(struct hrtimer *hrtimer)
>  {
>  	struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer);
> +	enum hrtimer_restart ret;
> +
> +	spin_lock_bh(&op->bcm_rx_update_lock);
>  
>  	if (bcm_rx_thr_flush(op)) {
>  		hrtimer_forward_now(hrtimer, op->kt_ival2);

[Severity: High]
This is a pre-existing issue, but could hrtimer_forward_now() be called with a
zero interval, triggering a WARN_ON_ONCE?

If a user updates an RX operation with msg_head->nframes == 0 and ival2 == 0,
bcm_rx_setup() updates op->kt_ival2 = 0 but does not clear op->last_frames.
It drops the lock before cancelling the timer:

net/can/bcm.c:bcm_rx_setup() {
    ...
    spin_lock_bh(&op->bcm_rx_update_lock);
    op->ival2 = msg_head->ival2;
    op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    ...
    spin_unlock_bh(&op->bcm_rx_update_lock);

    /* disable an active timer due to zero value? */
    if (!op->kt_ival1)
        hrtimer_cancel(&op->timer);

    hrtimer_cancel(&op->thrtimer);
    ...
}

Because hrtimer_cancel(&op->thrtimer) is called without the
bcm_rx_update_lock held, could a concurrently expiring bcm_rx_thr_handler()
acquire the lock, flush the still-throttled frame, and call
hrtimer_forward_now(hrtimer, op->kt_ival2) with an interval of 0?

> -		return HRTIMER_RESTART;
> +		ret = HRTIMER_RESTART;
>  	} else {
>  		/* rearm throttle handling */
>  		op->kt_lastmsg = 0;
> -		return HRTIMER_NORESTART;
> +		ret = HRTIMER_NORESTART;
>  	}
> +
> +	spin_unlock_bh(&op->bcm_rx_update_lock);
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=3