Re: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations

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

Pre-existing issues:
- [High] In `bcm_tx_setup()`, when updating an existing `bcm_op`, the CAN frames array (`op->frames`) is overwritten directly from userspace data using `memcpy_from_msg` without holding `bcm_tx_lock` and without using a staging buffer. This introduces a data race with concurrent transmission. Crucially, the direct overwrite stores user-provided frames into the active array before length validation. If an invalid length is provided, the function subsequently returns an error, but the corrupted frame remains active in `op->frames`. Concurrent or subsequent timer executions will transmit this malformed frame to the CAN driver, leading to out-of-bounds accesses and kernel panics.
- [Medium] Timer values in `bcm_tx_setup()` (`op->kt_ival1` and `op->kt_ival2`) are updated directly without holding `bcm_tx_lock`. Concurrently, `bcm_tx_timeout_handler()` running in softirq context reads these 64-bit `ktime_t` values without locks. On 32-bit architectures, this leads to torn reads, causing unpredictable timer behavior.
--

--- Patch [5]: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations ---
commit 74409df5d31bac1c2081de9ebaae45419726cd9c
Author: Oliver Hartkopp <[email protected]>

can: bcm: add missing rcu list annotations and operations

This commit addresses missing list_add_rcu() and list_del_rcu() calls in
the CAN broadcast manager to properly initialize and clean up bcm_op
structures. The operations are updated to protect concurrent procfs
traversals that use rcu_read_lock().

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 38c3f690c1cd0..940cd9df1cca3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1009,6 +1009,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	struct bcm_sock *bo = bcm_sk(sk);
>  	struct bcm_op *op;
>  	struct canfd_frame *cf;
> +	bool add_op_to_list = false;
>  	unsigned int i;
>  	int err;

[Severity: High]
This is a pre-existing issue, but does this code overwrite the CAN frames
directly from userspace before validating their lengths?

In bcm_tx_setup() when updating an existing bcm_op, the frames array is
updated directly without holding bcm_tx_lock and without a staging buffer:

bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    if (op->flags & CAN_FD_FRAME) {
        if (cf->len > 64)
            err = -EINVAL;
    ...
}

If an invalid length is provided, the function returns -EINVAL but the
corrupted frame remains active in op->frames.

Could concurrent or subsequent timer executions transmit this malformed frame
to the CAN driver, potentially triggering out-of-bounds accesses and panics?

[Severity: Medium]
This is a pre-existing issue, but are the timer values updated locklessly?

bcm_tx_setup() updates the 64-bit ktime_t variables kt_ival1 and kt_ival2
directly without holding bcm_tx_lock:

bcm_tx_setup() {
    ...
    op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
    op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
    ...
}

Concurrently, bcm_tx_timeout_handler() in softirq context reads these
values locklessly:

bcm_tx_timeout_handler() {
    ...
    if (op->kt_ival1 && (op->count > 0)) {
    ...
}

On 32-bit architectures, a 64-bit read/write is not atomic.

Could this result in torn reads where the timer handler sees a mix of old
and new 32-bit halves, leading to unpredictable timer behavior and erratic
transmission intervals?

[ ... ]

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