Re: [PATCH v15 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:

Pre-existing issues:
- [High] RCU List API Violation and Data Race in BCM operations list
- [High] Unprotected Data Race modifying CAN frames and filter state from userspace
--

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

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

Commit f1b4e32aca08 replaced synchronize_rcu() with call_rcu() and introduced
the RX_NO_AUTOTIMER flag. This patch resolves a thrtimer UAF by deferring
timer cancellation and memory free to a dedicated unbound workqueue. A
reference on op->sk is also taken to ensure the socket is not freed out from
under an armed timer.

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

[Severity: High]
This isn't a bug introduced by this patch, but does bcm_tx_setup() insert into
tx_ops using list_add() without RCU memory release barriers?

When bcm_proc_show() concurrently iterates tx_ops without RCU primitives, can
this missing dependency ordering lead to wild pointer dereferences?

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

bcm_rx_setup()
    list_add(&op->list, &bo->rx_ops);

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


[Severity: High]
This is a pre-existing issue, but do bcm_tx_setup() and bcm_rx_setup() modify
op->frames directly from user memory without holding their respective spinlocks?

If memcpy_from_msg() modifies op->frames while bcm_can_tx() reads them
concurrently in softirq context, can it result in the transmission of partially
written CAN frames?

bcm_tx_setup()
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

bcm_rx_setup()
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);

>  		op->ifindex = ifindex;
>  
>  		/* initialize uninitialized (kzalloc) structure */

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