Re: [PATCH 5/5] can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Lockless update of `op->frames` in `bcm_tx_setup()` creates a TOCTOU race allowing transmission of unvalidated CAN frames, leading to Out-of-Bounds reads in CAN drivers.
- [Critical] Complete lack of frame length validation in `bcm_rx_setup()` allows Out-of-Bounds reads when responding to RTR frames.
- [High] Lockless updates to 64-bit ktime_t timer variables in `bcm_tx_setup()` lead to torn reads and soft lockups on 32-bit platforms.
- [High] Objects are passed to call_rcu() while still linked in the RCU-protected tx_ops and rx_ops lists during socket release.
--

commit 0171fb1b05b0628e97f895bb66387fd0bd7335bb
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

sashiko-bot remarked the missing use of list_add_rcu() in
bcm_[rx|tx]_setup() to have a proper initialized bcm_op structure
when bcm_proc_show() traverses the bcm_op's under rcu_read_lock().

Link: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index f1f1f11d1f92a..6a1d52189c0ab 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1111,8 +1112,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		hrtimer_setup(&op->thrtimer, hrtimer_dummy_timeout, CLOCK_MONOTONIC,
>  			      HRTIMER_MODE_REL_SOFT);
>  
> -		/* add this bcm_op to the list of the tx_ops */
> -		list_add(&op->list, &bo->tx_ops);
> +		add_op_to_list = true;
>  
>  	} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */

[Severity: Critical]
This is a pre-existing issue, but does this lockless update of op->frames
create a race condition?

In bcm_tx_setup(), user data is copied directly into the active array:

err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

The validation (cf->len > 64 or > 8) happens after this copy without
holding op->bcm_tx_lock or disabling the active timer.

Could bcm_tx_timeout_handler() fire during this window, causing
bcm_can_tx() to read and transmit a partially updated or unvalidated
frame? If an attacker supplies an iovec that faults immediately after
the length byte is copied, leaving a malicious length (like 255), does
this cause an out-of-bounds read in the CAN driver when the timer
transmits the frame?

[Severity: High]
This is a pre-existing issue, but do the lockless updates to 64-bit timer
variables lead to torn reads on 32-bit platforms?

In bcm_tx_setup(), op->kt_ival1 and op->kt_ival2 are updated based on
user input without holding op->bcm_tx_lock or disabling softirqs.

Since bcm_tx_timeout_handler() reads these variables in softirq context:

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

Could a softirq interrupting the update read a torn ktime_t value (a mix
of old and new 32-bit halves) on 32-bit systems? Would passing this
garbage value to hrtimer_set_expires() result in the timer firing
continuously or hanging indefinitely?

> @@ -1134,6 +1134,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->flags |= TX_ANNOUNCE;
>  	}
>  
> +	/* add this bcm_op to the list of the tx_ops? */
> +	if (add_op_to_list)
> +		list_add_rcu(&op->list, &bo->tx_ops);
> +

[Severity: High]
This is a pre-existing issue, but does the teardown loop in bcm_release()
pass objects to call_rcu() while they are still linked in the RCU-protected
lists?

While this patch correctly updates list_add_rcu() for bo->tx_ops and
bo->rx_ops, bcm_release() iterates over these lists with
list_for_each_entry_safe() and calls bcm_remove_op().

Since bcm_remove_op() immediately calls call_rcu(&op->rcu, bcm_free_op_rcu),
the elements are never unlinked using list_del_rcu() before the call_rcu()
invocation.

Does leaving these objects physically linked in the RCU-protected list
violate the remove-before-reclaim ordering rule, potentially allowing
new readers to find the object after the grace period begins?

[ ... ]

> @@ -1376,10 +1377,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					      bcm_rx_handler, op, "bcm", sk);
>  		if (err) {
>  			/* this bcm rx op is broken -> remove it */
> -			list_del_rcu(&op->list);
>  			bcm_remove_op(op);
>  			return err;
>  		}
> +
> +		/* add this bcm_op to the list of the rx_ops */
> +		list_add_rcu(&op->list, &bo->rx_ops);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does the lack of frame length validation
in bcm_rx_setup() allow out-of-bounds reads during RTR replies?

When CAN frames are copied from user-space into op->frames via
memcpy_from_msg() in bcm_rx_setup(), the length field is never validated.

If the user configures the RX operation with the RX_RTR_FRAME flag,
bcm_rx_handler() automatically replies to incoming RTR packets:

if (op->flags & RX_RTR_FRAME) {
    if (rx_ext->skb->sk != op->sk)
        bcm_can_tx(op);
    return;
}

Could an attacker set an arbitrary length (e.g., 255) for
op->frames[0].len, causing bcm_can_tx() to send an unvalidated frame
and trigger an out-of-bounds heap read in the driver?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=5