Re: [PATCH v2 2/6] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe list insertions and non-RCU traversals in BCM operations lists.
- [High] Data Race and Data Corruption due to Lockless Modification of Active BCM Operations.
- [High] Corrupted BCM Operation State upon Validation Failure.
- [Medium] Unannotated lockless read of `bo->bound` and `bo->ifindex` in `bcm_notify()` during `NETDEV_DOWN`.
--

--- Patch [2]: [PATCH v2 2/6] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 0cd66671e00a6a20f593dd001c30f331b9c4d5d8
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

This commit fixes a race condition between bcm_sendmsg() and device
notification/connection by modifying the lockless fast-path and reading
ifindex under the socket lock. It also fixes a silent failure in
bcm_rx_setup() when the target device disappears.

Link: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/

> Re-validate under the socket lock: a concurrent bcm_notify()
> may have unbound this socket (device removal) after the
> lockless fast-path check above. bo->ifindex is only ever
> mutated under lock_sock(), so reading it here - instead of
> before taking the lock - guarantees it can't be observed
> torn against bo->bound.

[Severity: Medium]
This is a pre-existing issue, but is the commit message accurate about
bo->ifindex only being accessed under lock_sock()?

Looking at bcm_notify(), when processing NETDEV_DOWN, it reads bo->bound
and bo->ifindex locklessly:

    case NETDEV_DOWN:
        if (bo->bound && bo->ifindex == dev->ifindex) {

Since bcm_connect() mutates these while holding lock_sock(sk), could this
cause torn reads?

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index bdf53241bd7b3..b612135b017d1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  				op->rx_reg_dev = dev;
>  				dev_put(dev);
> +			} else {
> +				/* the requested device is gone - do not
> +				 * silently succeed without registering
> +				 */
> +				err = -ENODEV;
>  			}

[Severity: High]
This is a pre-existing issue, but are the list insertions for rx_ops and
tx_ops safe for concurrent lockless readers?

bcm_rx_setup() and bcm_tx_setup() insert into bo->rx_ops and bo->tx_ops.
Concurrently, bcm_proc_show() locklessly traverses bo->tx_ops under
rcu_read_lock() using the non-rcu list_for_each_entry() instead of
list_for_each_entry_rcu():

    list_for_each_entry(op, &bo->tx_ops, list) {

Could this lead to observing uninitialized list nodes and a system crash
during procfs traversal?

[Severity: High]
This is a pre-existing issue, but does bcm_rx_setup() safely modify active
filter frames?

When updating an existing bcm operation, it appears to directly overwrite
op->frames using memcpy_from_msg() without synchronizing against softirq
routines:

    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);

Since bcm_rx_handler() reads op->frames in softirq context, can it observe
partially written or torn can frames during the update?

[Severity: High]
This is a pre-existing issue, but what happens if validation fails during
an update?

In bcm_tx_setup() and bcm_rx_setup(), op->frames is directly overwritten.
If a subsequent frame fails validation (such as cf->len > 64 in
bcm_tx_setup()) or memcpy_from_msg() fails, the function returns an error
without reverting the state:

    if (err < 0)
        return err;

Does this leave the operation actively running on a corrupted buffer?

[ ... ]

> @@ -1535,7 +1564,12 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
>  				bo->bcm_proc_read = NULL;
>  			}
>  #endif
> -			bo->bound   = 0;
> +			/* Paired with the lockless fast-path check in
> +			 * bcm_sendmsg(); bo->ifindex itself is only ever
> +			 * accessed under lock_sock() so it needs no
> +			 * annotation.
> +			 */
> +			WRITE_ONCE(bo->bound, 0);
>  			bo->ifindex = 0;
>  			notify_enodev = 1;
>  		}

[Severity: Medium]
This is a pre-existing issue, but the comment here makes the same claim
that bo->ifindex is only accessed under lock_sock().

As noted above, does bcm_notify() read bo->bound and bo->ifindex locklessly
during the NETDEV_DOWN case?

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