Re: [PATCH v5 1/8] 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Invalid CAN frame length bypasses validation during BCM TX operation update, potentially causing driver-level out-of-bounds reads.
- [High] Missing RCU-safe list addition and traversal for tx_ops and rx_ops lists.
- [High] Data races when modifying existing CAN filters and frames concurrently with network rx/tx paths.
--

--- Patch [1]: [PATCH v5 1/8] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 19a74c15fd3018b3b148c40380ac7643f49b00f3
Author: Lee Jones <lee@kernel.org>

    can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
    
This commit replaces the direct call_rcu() with a deferred workqueue
item for freeing the bcm_op structure. This safely cancels the
timers and deallocates memory in sleepable process context, preventing
a use-after-free condition.

> 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: Critical]
This is a pre-existing issue, but does the frame length validation handle
invalid sizes correctly before they become active?

In bcm_tx_setup(), when updating an existing TX operation, user data is copied
directly into the active op->frames buffer before it is validated:

net/can/bcm.c:bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

    if (op->flags & CAN_FD_FRAME) {
        if (cf->len > 64)
            err = -EINVAL;
    } else {
        if (cf->len > 8)
            err = -EINVAL;
    }

    if (err < 0)
        return err;
    ...
}

If the provided cf->len is invalid (e.g., greater than 64 for CAN FD), the
function returns -EINVAL and exits, but the invalid length has already been
written into op->frames.

Since the existing op is still active and its timer continues ticking,
bcm_tx_timeout_handler() could eventually transmit this malformed frame to the
CAN device via can_send(). CAN drivers rely on the core stack to validate
lengths, and processing a length greater than 64 could cause out-of-bounds
array reads (e.g., in can_fd_len2dlc) or other memory corruption. Could an
unprivileged user sending a crafted payload trigger this?

[Severity: High]
This isn't a bug introduced by this patch, but does this code need to use
list_add_rcu() to ensure safe concurrent traversal?

The tx_ops and rx_ops lists are iterated in bcm_proc_show() under
rcu_read_lock() without holding the socket lock:

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

However, new operations are inserted into these lists in bcm_tx_setup() and
bcm_rx_setup() using the standard list_add() macro:

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

On weakly-ordered CPU architectures, this lacks the necessary
smp_store_release() memory barriers, meaning concurrent RCU readers in
bcm_proc_show() can observe a new list node before its contents or its next
pointer are fully initialized, potentially leading to data corruption or
crashes.

Furthermore, bcm_proc_show() iterates over tx_ops using list_for_each_entry()
rather than list_for_each_entry_rcu(), omitting the data dependency barriers
required for safe concurrent traversal. Can userspace reading
/proc/net/can-bcm/* while configuring a socket trigger this?

[Severity: High]
This isn't a bug introduced by this patch, but are there data races when
updating existing CAN filters and frames concurrently?

In bcm_rx_setup() and bcm_tx_setup(), when updating an existing bcm_op, the
user-provided data is copied directly into op->frames without holding
bcm_tx_lock or any lock that synchronizes with the network datapath:

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

    memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
    ...
}

Concurrently, bcm_rx_handler() (running in softirq context) iterates over
op->frames and reads/modifies op->last_frames locklessly:

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

This could lead to memory corruption in last_frames or matching against
partially updated frames.

Similarly, bcm_can_tx() retrieves a pointer into op->frames under
bcm_tx_lock, but drops the lock prior to calling skb_put_data() (which reads
from op->frames):

net/can/bcm.c:bcm_can_tx() {
    ...
    skb_put_data(skb, cf, op->cfsiz);
    ...
}

Does this allow a data race where a CAN frame is being transmitted at the
exact same time it is being overwritten?

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

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