Re: [PATCH v2 3/6] can: bcm: add locking when updating filter and timer values

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The TX timeout handler reads 64-bit ktime_t variables and 32-bit state variables locklessly, creating data races that can lead to torn ktime_t reads and erratic timer behavior.
- [High] The CAN frame payload on the transmission side is copied into the SKB locklessly, allowing a concurrent update from userspace to overwrite the frame while it is being transmitted, resulting in torn CAN frames.
--

--- Patch [3]: [PATCH v2 3/6] can: bcm: add locking when updating filter and timer values ---
commit 5cea5eef9652435aa644979477f4e925e5e24c8f
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

This commit adds a new per-op bcm_rx_update_lock to protect timer and
filter data updates in the RX handlers against concurrent execution. It
also addresses an RTR flag clearing race by normalizing the frame content
during initial preparation instead of modifying the live frame.

Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..1eabacad0b001 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -293,22 +294,23 @@ 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;
>  	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 (!cf) {
> +		/* 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);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to torn CAN
frames during transmission?

While the bcm_tx_lock protects the calculation of the cf pointer here, the
lock is dropped before calling skb_put_data(skb, cf, op->cfsiz) further
down in bcm_can_tx().

Concurrently, bcm_tx_setup() can overwrite the active op->frames buffer
using memcpy_from_msg() directly from userspace locklessly:

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

Could this allow a partially updated CAN frame to be copied into the skb and
transmitted onto the network while the data is being mutated?

[ ... ]
> @@ -461,7 +463,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>  	struct bcm_msg_head msg_head;
>  
>  	if (op->kt_ival1 && (op->count > 0)) {

[Severity: High]
This is a pre-existing issue, but can the lockless read of op->kt_ival1
result in a torn read on 32-bit platforms?

In bcm_tx_setup(), the 64-bit ktime_t variables kt_ival1 and kt_ival2 can
be updated concurrently from user space data. Evaluating these values
in bcm_tx_timeout_handler() and bcm_tx_set_expiry() without holding
bcm_tx_lock could yield arbitrary timer intervals.

Could this cause the hrtimer to stall indefinitely or fire in an immediate
tight loop?

> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);
>  		if (!op->count && (op->flags & TX_COUNTEVT)) {
>  
>  			/* create notification to user */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net?part=3