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

On 14.07.26 12:39, [email protected] wrote:
> 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.
> 

Fixed later in patch 5.

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