Re: [PATCH v4 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless reads of timer and state variables in `bcm_tx_timeout_handler()` cause torn reads and TOCTOU bugs.
--

--- Patch [3]: [PATCH v4 3/7] can: bcm: add locking when updating filter and timer values ---
commit 59929132ac1eda994b23441375ebf1eecf8fc14d
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.

Reported-by: syzbot+75e5e4ae00c3b4bb544e@syzkaller.appspotmail.com
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..b9b6f1b7bb03b 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -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 while the patch adds locking to protect
timers in the RX path, it looks like a similar data race might exist here
in the TX path.

Can lockless reads of op->kt_ival1, op->kt_ival2, and op->count in
bcm_tx_timeout_handler() lead to torn reads on 32-bit systems?

Since bcm_tx_setup() updates these 64-bit timer intervals locklessly:

net/can/bcm.c:bcm_tx_setup() {
    ...
    if (op->flags & SETTIMER) {
        ...
        op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
        op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    }
    ...
}

And bcm_tx_set_expiry() also reads them locklessly:

net/can/bcm.c:bcm_tx_set_expiry() {
    ...
    if (op->kt_ival1 && op->count)
        ival = op->kt_ival1;
    else if (op->kt_ival2)
        ival = op->kt_ival2;
    ...
}

Would it be possible for the hrtimer softirq context to see torn ktime_t
values if it preempts or runs concurrently with bcm_tx_setup()?

Could this lead to wildly incorrect timer expiry values that might stall
the cyclic transmission, or corrupt user notifications?

> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {
>  
>  			/* create notification to user */

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