Re: [PATCH v10 01/10] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Inconsistent use of RCU list primitives can lead to data races and crashes when reading `/proc/net/can-bcm/*`.
- [High] Data race on `op->frames` during runtime updates in `bcm_tx_setup()` and `bcm_rx_setup()`.
--

--- Patch [1]: [PATCH v10 01/10] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 07fedad11f0f52be9c8ac130e930be315886f42d
Author: Lee Jones <lee@kernel.org>

can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

Commit f1b4e32aca08 replaced synchronize_rcu() with call_rcu() but
omitted flag checks for thrtimer, creating a TOCTOU race and UAF.
This resolves the race by deferring timer cancellation and memory free
to a dedicated unbound workqueue, which avoids sleeping in RCU callbacks.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..bdf53241bd7b3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1060,6 +1081,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  		/* bcm_can_tx / bcm_tx_timeout_handler needs this */
>  		op->sk = sk;
> +		sock_hold(sk);
>  		op->ifindex = ifindex;
>  
>  		/* initialize uninitialized (kzalloc) structure */

[Severity: High]
This is a pre-existing issue, but I noticed it while reviewing the op setup
changes. Does the list insertion in bcm_tx_setup() and bcm_rx_setup() need
to use list_add_rcu() instead of list_add()?

It looks like bcm_tx_setup() adds to the list like this:

net/can/bcm.c:bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
    ...
}

Since readers like bcm_proc_show() iterate the tx_ops list inside an RCU
read-side critical section, could the lack of memory barriers in list_add()
allow a lockless reader to observe a partially initialized list node?
Also, bcm_proc_show() appears to use the non-RCU list_for_each_entry()
iterator.

[Severity: High]
This isn't a bug introduced by this patch, but there might be a data race on
op->frames during runtime updates.

In bcm_tx_setup() and bcm_rx_setup(), it appears the memory content of
op->frames can be updated directly from userspace messages:

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

Does this allow concurrent readers to access partially updated memory?
For example, bcm_can_tx() calculates the frame pointer under op->bcm_tx_lock
but then accesses the memory content via skb_put_data() outside the lock.

Could this result in torn CAN frames being transmitted, or incorrect rx
filtering if a concurrent bcm_rx_handler() reads op->frames while it is
being modified?

[ ... ]

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