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

On 14.07.26 12:32, [email protected] wrote:
> 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.
> --
> 

Fixed in patch 6

> --- 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?
> 
> [ ... ]
>