Re: [PATCH v6 1/9] 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:

New issues:
- [High] Unregistering the CAN protocol before flushing the workqueue causes a kernel WARNING and leaks the slab cache during module unload.

Pre-existing issues:
- [High] Unsafe lockless iteration of `tx_ops` and `rx_ops` lists without RCU wrappers leads to data races and potential kernel crashes.
- [High] Unprotected data races during CAN frame configuration updates against concurrent timers and softirq handlers.
--

--- Patch [1]: [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 31beeaafb4b7e3f7496ed0979679240bbb843e27
Author: Lee Jones <lee@kernel.org>
Subject: can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

This commit replaces immediate call_rcu() with workqueue deferral for BCM
operations to safely cancel hrtimers that can sleep, fixing a use-after-free.
It adds a reference count on op->sk to prevent the socket from disappearing
before the workqueue executes.

> 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;

[Severity: High]
This is a pre-existing issue, but does lockless list iteration of tx_ops
and rx_ops miss RCU wrappers?

When reading /proc/net/can-bcm/* via bcm_proc_show(), the code iterates
over tx_ops locklessly using the non-RCU list_for_each_entry().

Concurrently, operations added in bcm_tx_setup() (and bcm_rx_setup() for
rx_ops) use the standard list_add() instead of list_add_rcu():

bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
}

Since list_add() omits the smp_store_release() memory barrier, can a
concurrent reader see uninitialized or partially linked list pointers,
leading to page faults or infinite loops?

[ ... ]

> @@ -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;

[Severity: High]
This is a pre-existing issue, but are CAN frame configuration updates
unprotected against concurrent softirq handlers and timers?

In bcm_rx_setup(), memcpy_from_msg() directly modifies op->frames and
op->last_frames without synchronization. Concurrently, bcm_rx_handler()
accesses these frames locklessly in softirq context:

bcm_rx_handler() {
    ...
    if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==
        (get_u64(op->frames, 0) &
         get_u64(op->frames + op->cfsiz * i, 0))) {
    ...
}

Similarly in bcm_tx_setup(), memcpy_from_msg() writes user data directly
into op->frames while a firing timer could trigger bcm_can_tx(), which
reads from op->frames locklessly.

Could this result in torn reads and cause the transmission or processing of
corrupted or partially validated CAN frames?

[ ... ]

> @@ -1869,6 +1898,8 @@ static void __exit bcm_module_exit(void)
>  	can_proto_unregister(&bcm_can_proto);
>  	unregister_netdevice_notifier(&canbcm_notifier);
>  	unregister_pernet_subsys(&canbcm_pernet_ops);
> +	rcu_barrier();
> +	destroy_workqueue(bcm_wq);
>  }

[Severity: High]
Can the CAN protocol unregister before the workqueue is flushed?

If a user creates a BCM socket, arms a timer, and closes the socket, and then
the module is immediately unloaded, bcm_module_exit() executes
can_proto_unregister() before destroy_workqueue().

At this point, the deferred bcm_free_op_work() may still be pending in the
workqueue holding a reference to the socket.

Does this cause can_proto_unregister() to destroy the socket's slab cache
while it is still populated, leaking the cache and causing a potential
memory corruption when sk_free() falls back to kfree() on a slab-allocated
object?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=1