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

Pre-existing issues:
- [High] Missing list_add_rcu() when inserting into tx_ops and rx_ops causes RCU publish/subscribe rules violation, leading to data races and potential kernel panics with concurrent procfs readers.
- [High] Lockless updates to 64-bit ktime_t fields can cause torn reads on 32-bit architectures, leading to wildly incorrect timer timeouts.
- [High] Unprotected concurrent modification of `op->frames` via `memcpy_from_msg` in `bcm_rx_setup` and `bcm_tx_setup`.
--

--- Patch [1]: [PATCH v2 1/6] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 66477b55911323fb4b8a9cc14c9ed91e1fa4bfa5
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 a 
flag check in the fast-path, causing a use-after-free when thrtimer fires. 
This patch defers timer cancellation and memory deallocation to a dedicated 
unbound workqueue to safely sleep and drop socket references.

Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")

> ---
> net/can/bcm.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> 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 does the list insertion in bcm_tx_setup() 
and bcm_rx_setup() use list_add() instead of list_add_rcu()?

If a local unprivileged user repeatedly adds new TX/RX operations while 
concurrently reading /proc/net/can-bcm/<inode>, can bcm_proc_show() observe 
partially initialized bcm_op entries? 

bcm_proc_show() iterates over rx_ops using list_for_each_entry_rcu() and 
tx_ops using list_for_each_entry() without holding the socket lock. Could 
the missing smp_store_release() barrier in list_add() lead to torn reads of 
uninitialized list pointers and potential kernel panics?

[Severity: High]
This isn't a bug introduced by this patch, but are the 64-bit ktime_t 
fields (op->kt_ival1 and op->kt_ival2) updated locklessly in bcm_tx_setup() 
and bcm_rx_setup()?

On 32-bit architectures, can these lockless updates cause torn reads when 
bcm_tx_timeout_handler() or bcm_rx_timeout_handler() concurrently read 
these fields in softirq context to arm the hrtimer? 

Could this lead to wildly incorrect timer expirations, potentially stalling 
CAN transmissions indefinitely or flooding the bus?

[ ... ]

> @@ -1221,6 +1243,7 @@ static int bcm_rx_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;
>  
>  		/* ifindex for timeout events w/o previous frame reception */

[Severity: High]
This is a pre-existing issue, but is it safe to write directly into the 
active op->frames using memcpy_from_msg() without holding a lock?

When updating an existing BCM operation, bcm_tx_setup() and bcm_rx_setup() 
appear to copy user data directly into op->frames locklessly. Because 
memcpy_from_msg() can sleep on user faults, is there a significant race 
window here?

Concurrently, bcm_can_tx() reads op->frames to transmit data, and 
bcm_rx_cmp_to_index() reads op->frames to filter incoming packets. Could 
this allow partially updated or torn CAN frames to be transmitted onto the 
physical CAN bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net?part=1