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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Torn read data races on timer values in the TX path.
- [High] The lock added in bcm_can_tx() is ineffective against torn reads because the writer (bcm_tx_setup) does not hold the lock.
- [High] Timer armed on freed bcm_op due to race between bcm_rx_handler and bcm_delete_rx_op.
- [High] Missing list_add_rcu() for BCM operations accessed locklessly via RCU.
- [High] Unsafe lockless traversal of tx_ops list in procfs handler.
- [Medium] op->rx_stamp, op->rx_ifindex, and op->frames_abs are updated locklessly in bcm_rx_handler().
--

--- Patch [3]: [PATCH v8 03/10] can: bcm: add locking when updating filter and timer values ---
commit ae43444642bccb605ce6673ee360da9626ce6a3b
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

This commit adds a new bcm_rx_update_lock to protect timer and filter data
updates in bcm_rx_setup() against concurrent execution of RX handlers.
It also modifies the transmit paths and timer handlers to attempt to close
several synchronization races.

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..af4bb4182bc29 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> -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;
[ ... ]
> +	if (!cf) {
> +		/* read currframe under lock protection */
> +		spin_lock_bh(&op->bcm_tx_lock);
> +		memcpy(&cframe, op->frames + op->cfsiz * op->currframe,
> +		       op->cfsiz);
> +		cf = &cframe;
> +		spin_unlock_bh(&op->bcm_tx_lock);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but is this lock effective
against torn reads here?

The writer in bcm_tx_setup() modifies op->frames locklessly via
memcpy_from_msg():

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

Since the writer mutates the buffer without holding bcm_tx_lock, does the
reader's lock in bcm_can_tx() prevent a concurrently firing timer callback
from reading and transmitting a corrupted, partially-overwritten CAN frame?

[ ... ]
> @@ -461,7 +466,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)) {
> -		bcm_can_tx(op);
> +		bcm_can_tx(op, NULL);

[Severity: High]
This is a pre-existing issue, but can we experience torn reads on these
timer values?

In bcm_tx_setup(), the timer fields are updated locklessly:

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() and bcm_tx_set_expiry() read these
fields locklessly.

On 32-bit systems, reading or writing a 64-bit ktime_t is not atomic.
Could a torn read (where half of the old ktime and half of the new one
are read) result in a corrupted timer expiration time being programmed?

[ ... ]
> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  	/* update statistics */
>  	op->frames_abs++;
>  
> -	if (op->flags & RX_RTR_FRAME) {
> +	/* snapshot the flag under lock: op->flags/op->frames may be updated
> +	 * concurrently by bcm_rx_setup().
> +	 */
> +	spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: Medium]
This isn't a bug introduced by this patch, but does the lockless update of
metadata before taking bcm_rx_update_lock cause mismatched data?

Right before this snippet, bcm_rx_handler() updates metadata locklessly:

bcm_rx_handler()
    op->rx_stamp = skb->tstamp;
    op->rx_ifindex = skb->dev->ifindex;
    op->frames_abs++;

For a socket bound to all interfaces (ifindex == 0), bcm_rx_handler() can
execute concurrently on multiple CPUs for the same bcm_op.

Since bcm_send_to_user() reads these values inside the lock, could it bundle
a CAN frame from one CPU with the timestamp or ifindex metadata from another?

[ ... ]
> @@ -778,6 +811,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  
>  rx_starttimer:
>  	bcm_rx_starttimer(op);
> +
> +	spin_unlock_bh(&op->bcm_rx_update_lock);
>  }

[Severity: High]
This is a pre-existing issue, but can this arm the timer on a freed object?

The teardown path in bcm_delete_rx_op() sets the flag locklessly and
cancels the timer:

bcm_delete_rx_op()
    op->flags |= RX_NO_AUTOTIMER;
    ...
    hrtimer_cancel(&op->timer);

Since bcm_delete_rx_op() doesn't take bcm_rx_update_lock, it can cancel
the timer before the handler arms it.

If bcm_rx_handler() reads op->flags before it is set, and then reaches
bcm_rx_starttimer(), could it start the timer on an object that will be
freed after the RCU grace period, causing a use-after-free?

[ ... ]
> @@ -1116,7 +1151,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);

[Severity: High]
This is a pre-existing issue, but is the lockless traversal of tx_ops safe?

bcm_tx_setup() adds operations to the list using list_add():

bcm_tx_setup()
    list_add(&op->list, &bo->tx_ops);

However, bcm_proc_show() iterates this list locklessly using the non-RCU
macro list_for_each_entry() while holding only rcu_read_lock().

Since the procfs reader does not use the proper RCU primitives for tx_ops,
could it follow corrupted pointers if the list is modified concurrently?

[ ... ]
> @@ -1164,6 +1217,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	/* check the given can_id */
>  	op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
>  	if (op) {
> +		void *new_frames = NULL;
> +

[Severity: High]
This is a pre-existing issue, but does rx_ops require list_add_rcu()?

Later in bcm_rx_setup(), it inserts the struct into the rx_ops list without
RCU memory barriers:

bcm_rx_setup()
    list_add(&op->list, &bo->rx_ops);

Meanwhile, bcm_proc_show() iterates the list expecting RCU protection:

bcm_proc_show()
    list_for_each_entry_rcu(op, &bo->rx_ops, list) { ... }

Because list_add() lacks the smp_store_release() semantics of
list_add_rcu(), could an RCU reader observe the new list node before its
memory contents are fully visible across cores?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v8-0-8c2f5f3bc212@hartkopp.net?part=3