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

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

Pre-existing issues:
- [High] The `/proc/net/can-bcm` read path contains data races when iterating over `bo->rx_ops` and `bo->tx_ops`, leading to a potential kernel panic (DoS).
--

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

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

This commit resolves a use-after-free in the CAN BCM module by deferring
the timer cancellation and memory deallocation of bcm_op to a dedicated
workqueue. It introduces flag checks and takes a reference on the socket
to ensure it outlives the deferred work.

> 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 isn't a bug introduced by this patch, but does the current code lack
proper RCU publish semantics when inserting new BCM operations?

While looking at bcm_tx_setup(), the new operation seems to be published
using list_add():

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

Similarly, bcm_rx_setup() appears to use list_add():

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

If bcm_proc_show() iterates over these lists while holding only
rcu_read_lock(), could this lead to uninitialized memory accesses or a
kernel panic (DoS)?

For example, bcm_proc_show() traverses tx_ops using a non-RCU iterator:

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

Since list_add() lacks the smp_store_release() memory barrier provided by
list_add_rcu(), and the traversal in bcm_proc_show() lacks READ_ONCE() and
rcu_dereference() protections, could this expose the traversal to concurrent
list mutations?

An unprivileged user could potentially trigger this by continuously reading
/proc/net/can-bcm/<inode> while another thread repeatedly invokes sendmsg()
with TX_SETUP/RX_SETUP and TX_DELETE/RX_DELETE opcodes to modify the lists.

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