Re: [PATCH v4 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent procfs reads can crash the kernel due to unsafe lockless list operations on `bo->rx_ops` and `bo->tx_ops`.
--

--- Patch [1]: [PATCH v4 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 784fd8a5d6989051af26ae5075b37f80eb5ecce7
Author: Lee Jones <lee@kernel.org>

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

This commit resolves a use-after-free issue by deferring the cancellation
of timers and the deallocation of BCM operations to an unbound workqueue.
It takes an additional socket reference to ensure the socket 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 is a pre-existing issue, but can concurrent procfs reads crash the kernel
due to unsafe lockless list operations on bo->tx_ops?

Later in bcm_tx_setup(), the new op is added to the list:

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

However, bcm_proc_show() locklessly iterates bo->tx_ops using the non-RCU
macro list_for_each_entry() instead of list_for_each_entry_rcu(), and
bcm_tx_setup() uses list_add() instead of list_add_rcu(). Because list_add()
lacks the smp_store_release() memory barrier of list_add_rcu(), and because
the tx_ops traversal lacks rcu_dereference() to enforce data dependencies,
the reader CPU can observe the new list node before its fields or next
pointer are initialized.

Could this lead to dereferencing uninitialized memory or a corrupted next
pointer if a local user repeatedly reads /proc/net/can-bcm/<inode> while a
thread creates new TX operations via sendmsg()?

[ ... ]

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

[Severity: High]
This is also a pre-existing issue, but a similar hazard exists for bo->rx_ops.

Later in bcm_rx_setup(), the code inserts into the list without RCU barriers:

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

While bcm_proc_show() does use list_for_each_entry_rcu() for rx_ops (unlike
tx_ops), bcm_rx_setup() still uses list_add() instead of list_add_rcu().
Without the smp_store_release() memory barrier provided by list_add_rcu(),
concurrent procfs reads can still observe the new node before its contents
are fully initialized.

Should these insertions be upgraded to use list_add_rcu() to match the
lockless RCU iteration in procfs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v4-0-aa87b8b9d33f@hartkopp.net?part=1