Re: [PATCH v15 03/11] can: bcm: add locking when updating filter and timer values

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

Pre-existing issues:
- [High] Missing validation of the `len` field for user-provided CAN frames when creating an `RX_RTR_FRAME` filter leads to an out-of-bounds read and memory leak onto the CAN bus.
- [High] Data race on `op->frames_abs` leads to a divide by zero panic in procfs.
- [High] Lockless updates to 64-bit timer values and state fields in `bcm_tx_setup()` cause data races and torn reads.
- [High] `bcm_tx_setup()` updates `op->frames` without lock, making the reader's lock in `bcm_can_tx()` useless and transmitting torn CAN frames.
--

--- Patch [3]: [PATCH v15 03/11] can: bcm: add locking when updating filter and timer values ---
commit c68765d3cdec02f986dd3eafa2bbc8dffcaae484
Author: Oliver Hartkopp <[email protected]>

    can: bcm: add locking when updating filter and timer values
    
    KCSAN detected a simultaneous access to timer values that can be
    overwritten in bcm_rx_setup() when updating timer and filter content
    while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
    run concurrently on incoming CAN traffic.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..1e5f8d65d351b 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -293,22 +294,28 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>   * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>   *              of the given bcm tx op
>   */
> -static void bcm_can_tx(struct bcm_op *op)
> +static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
>  {
>  	struct sk_buff *skb;
>  	struct can_skb_ext *csx;
>  	struct net_device *dev;
> -	struct canfd_frame *cf;
> +	struct canfd_frame cframe;
> +	bool cyclic = !cf;
> +	unsigned int idx = 0;
>  	int err;
>  
>  	/* no target device? => exit */
>  	if (!op->ifindex)
>  		return;
>  
> -	/* read currframe under lock protection */
> -	spin_lock_bh(&op->bcm_tx_lock);
> -	cf = op->frames + op->cfsiz * op->currframe;
> -	spin_unlock_bh(&op->bcm_tx_lock);
> +	if (cyclic) {
> +		/* read currframe under lock protection */
> +		spin_lock_bh(&op->bcm_tx_lock);
> +		idx = op->currframe;
> +		memcpy(&cframe, op->frames + op->cfsiz * idx, op->cfsiz);
> +		cf = &cframe;
> +		spin_unlock_bh(&op->bcm_tx_lock);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does taking bcm_tx_lock here
actually protect against torn reads during concurrent updates?

Looking at bcm_tx_setup(), the user-provided frames are copied directly into
op->frames without holding bcm_tx_lock:

bcm_tx_setup() {
    ...
    for (i = 0; i < msg_head->nframes; i++) {
        cf = op->frames + op->cfsiz * i;
        err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Since the writer does not acquire the lock, a cyclic transmission firing in
bcm_tx_timeout_handler() might read partially updated bytes while
memcpy_from_msg() is executing. Could this lead to the transmission of
corrupted, torn CAN frames to the CAN bus?

[ ... ]
> @@ -341,14 +348,20 @@ static void bcm_can_tx(struct bcm_op *op)
>  	if (!err)
>  		op->frames_abs++;

[Severity: High]
This is a pre-existing issue, but can this lockless increment of
op->frames_abs lead to a division by zero panic in procfs?

In bcm_rx_changed(), when op->frames_filtered exceeds ULONG_MAX/100, both
variables are locklessly cleared to 0:

bcm_rx_changed() {
    ...
    if (op->frames_filtered > ULONG_MAX/100)
        op->frames_filtered = op->frames_abs = 0;
    ...
}

Concurrently, a user reading /proc/net/can-bcm executes bcm_proc_show(),
which checks if op->frames_abs is non-zero before calculating a percentage:

bcm_proc_show() {
    ...
    if (!op->frames_abs)
        continue;
    ...
    seq_printf(...);
    ...
    reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
    ...
}

Because of the opaque function call to seq_printf(), the compiler must
reload op->frames_abs from memory for the division. If bcm_rx_changed()
clears it to 0 precisely after the initial check, would this result in
a divide by zero?

[ ... ]
> @@ -1116,7 +1159,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	}
>  
>  	if (op->flags & TX_ANNOUNCE)
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  
>  	if (op->flags & STARTTIMER)
>  		bcm_tx_start_timer(op);

[Severity: High]
This is a pre-existing issue, but does this code leave the timer updates in
bcm_tx_setup() exposed to data races?

In bcm_tx_setup(), the timer values are updated directly without holding
bcm_tx_lock or any other synchronization:

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

Simultaneously, the bcm_tx_timeout_handler() softirq might fire and read
these exact fields locklessly to dispatch timeout notifications to userspace.

Could this lead to torn reads of 64-bit values (like ktime_t) on 32-bit
systems, leaking corrupted data to userspace and causing incorrect timer
behavior?

> @@ -1130,6 +1173,24 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	return err;
>  }
>  
> +static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
> +				   struct bcm_op *op, void *new_frames)
> +{
> +	/* funny feature in RX(!)_SETUP only for RTR-mode:
> +	 * copy can_id into frame BUT without RTR-flag to
> +	 * prevent a full-load-loopback-test ... ;-]
> +	 * normalize this on the staged buffer, before it is
> +	 * ever installed into op->frames.
> +	 */
> +	if (msg_head->flags & RX_RTR_FRAME) {
> +		struct canfd_frame *frame0 = new_frames;
> +
> +		if ((msg_head->flags & TX_CP_CAN_ID) ||
> +		    frame0->can_id == op->can_id)
> +			frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but does this function omit the bounds check
on the user-provided len field?

Unlike bcm_tx_setup() which properly validates the length, this code normalizes
the RTR frame without validating frame0->len.

When a matching remote request is received, the malformed frame is
transmitted by bcm_rx_handler() -> bcm_can_tx(). The CAN driver then reads
len bytes from the frame.

If the len field was maliciously set (e.g., 255), could this result in an
out-of-bounds read past the end of the sk_buff payload, leading to a memory
leak of adjacent kernel memory onto the CAN bus?

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