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

Pre-existing issues:
- [High] Missing list_add_rcu and list_for_each_entry_rcu for BCM operations lists.
--

--- Patch [1]: [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 3bf6c2fae3452e20dc5bcc0bb3d49f9026358964
Author: Lee Jones <lee@kernel.org>

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

This commit replaces direct hrtimer cancellation in softirq context with
a deferred workqueue approach for BCM rx_op deallocation. It queues a work
item from the RCU callback and holds a socket reference until timers are
fully cancelled to avoid a sleep-in-atomic panic.

> 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 since this patch modifies the BCM op
initialization, I noticed a potential race condition with lockless readers.

Later in bcm_tx_setup() and bcm_rx_setup(), the code adds the newly
initialized op to the socket's operation lists using the standard list API:

    list_add(&op->list, &bo->tx_ops);

Similarly, bcm_proc_show() iterates over bo->tx_ops locklessly using the
standard list_for_each_entry() macro:

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

Because list_add() does not provide smp_store_release() semantics, could a
CPU reorder the initialization of the op's fields or its next pointer?

If this happens, could a concurrent reader in bcm_proc_show() observe a NULL
next pointer or uninitialized object data, potentially causing a crash when
the procfs file is read?

Would it be safer to use list_add_rcu() here and list_for_each_entry_rcu()
for the lockless readers to ensure correct dependency barriers?

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