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

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

New issues:
- [High] Module exit sequence destroys the socket slab cache before flushing the deferred workqueue, causing a kernel warning and slab cache leak.

Pre-existing issues:
- [High] Missing RCU-safe list primitives for rx_ops and tx_ops list modifications and traversals, causing potential crashes during procfs reads.
--

--- Patch [1]: [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 9beb61cd8491cca022797eead8556214843e4d99
Author: Lee Jones <[email protected]>

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

This commit fixes a use-after-free by deferring the deallocation of
bcm_op to a dedicated workqueue instead of using call_rcu(). It ensures
that hrtimer cancellation and memory freeing occur in a sleepable context.
It also takes a reference on the socket while the work is pending to
prevent the socket from being freed prematurely.

> 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 can traversing the RCU-protected tx_ops
and rx_ops lists without RCU-safe primitives cause crashes during procfs
reads?

Looking at bcm_tx_setup() and bcm_rx_setup(), list_add() is used instead
of list_add_rcu(), which lacks the smp_store_release() memory barriers:

bcm_tx_setup() {
    ...
    /* add this bcm_op to the list of the tx_ops */
    list_add(&op->list, &bo->tx_ops);
    ...
}

Concurrently, if another thread reads /proc/net/can-bcm, bcm_proc_show()
traverses bo->tx_ops locklessly under rcu_read_lock() using the non-RCU
list_for_each_entry():

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

Can weakly ordered CPUs reorder the initialization of the bcm_op structure
after its insertion, allowing lockless RCU readers to observe uninitialized
memory and crash?

[ ... ]

> @@ -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]
Does this module exit sequence destroy the socket slab cache before flushing
the deferred workqueue?

Looking at bcm_free_op_work(), the deferred work item drops the final socket
reference:

bcm_free_op_work() {
    ...
    sock_put(op->sk);
    kfree(op);
}

If there are pending work items in bcm_wq during module unload,
can_proto_unregister() will call kmem_cache_destroy() on the protocol's
slab cache while sockets are still allocated.

Will this cause a kernel warning and a slab cache leak, since
destroy_workqueue() runs after can_proto_unregister() and will attempt to
free sockets into the destroyed slab?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1