Re: [PATCH v11 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] Data race when updating CAN frames of an existing BCM RX operation
- [High] Missing RCU list primitives causes data races with lockless procfs readers
--

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

This commit fixes a thrtimer UAF during BCM RX operation teardown by
deferring timer cancellation and memory deallocation to a dedicated unbound
workqueue. It also retains a reference on op->sk until the deferred work
completes to ensure the socket is not 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

[ ... ]

> @@ -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 does bcm_rx_setup() have a data race when
updating CAN frames for an existing BCM RX operation?

It appears memcpy_from_msg() copies directly from user space into op->frames
without holding op->bcm_rx_update_lock or using a staging buffer:

net/can/bcm.c:bcm_rx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    ...
    memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
    ...
}

Since memcpy_from_msg() can sleep, it cannot hold the lock, but this means
concurrent softirq readers like bcm_rx_handler() might observe partially
updated filter masks:

net/can/bcm.c:bcm_rx_handler() {
    ...
    if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==
    ...
}

Could this lead to valid CAN frames being incorrectly dropped or invalid
frames accepted?

[Severity: High]
This is a pre-existing issue, but does bcm_rx_setup() lack proper RCU list
primitives when inserting into bo->rx_ops?

It currently uses the non-RCU list_add() during setup:

net/can/bcm.c:bcm_rx_setup() {
    ...
    list_add(&op->list, &bo->rx_ops);
    ...
}

This lacks the smp_store_release() barrier needed for concurrent lockless
readers. For instance, bcm_proc_show() iterates these lists locklessly
under rcu_read_lock(), but it uses list_for_each_entry() instead of
list_for_each_entry_rcu():

net/can/bcm.c:bcm_proc_show() {
    ...
    list_for_each_entry(op, &bo->tx_ops, list) {
    ...
}

Can this lack of memory ordering and dependency barriers allow the compiler
to generate unsafe traversals, potentially leading to a NULL pointer
dereference or invalid list traversal for concurrent procfs readers of
/proc/net/can-bcm/*?

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