Re: [PATCH v9 02/10] 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 lockless list traversal and missing memory barriers for tx_ops and rx_ops lists.
- [High] Unbounded allocation of bcm_op structures allows local users to exhaust kernel memory.
- [Medium] bcm_rx_setup() silently fails to resume frame delivery for an updated RX operation if the target network device was previously unregistered.
- [Medium] Data race on bo->ifindex and bo->bound during NETDEV_DOWN handling risks torn reads and spurious errors.
--

--- Patch [2]: [PATCH v9 02/10] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 48dc89358d05a6bfc0097f061749b853a77c5328
Author: Oliver Hartkopp <socketcan@hartkopp.net>
Subject: can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

This commit fixes a data race between lockless reads of bo->bound and
bo->ifindex in bcm_sendmsg() and locked updates in bcm_notify() and other
paths. It also addresses a silent failure in bcm_rx_setup() when the
requested target device concurrently disappears.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> --- 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 does the current iteration logic for
bo->tx_ops and bo->rx_ops in bcm_proc_show() risk wild memory accesses or
infinite loops?

bcm_proc_show() iterates over the operations holding only rcu_read_lock():

net/can/bcm.c:bcm_proc_show() {
    ...
    list_for_each_entry_rcu(op, &bo->rx_ops, list) { ... }
    ...
    list_for_each_entry(op, &bo->tx_ops, list) { ... }
}

Meanwhile, bcm_rx_setup() and bcm_tx_setup() insert new elements without
an RCU write memory barrier:

net/can/bcm.c:bcm_rx_setup() {
    ...
    /* add this bcm_op to the list of the rx_ops */
    list_add(&op->list, &bo->rx_ops);
}

Could this allow a local user reading /proc/net/can-bcm to crash the
system while operations are concurrently registered?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for local
users to exhaust kernel memory by creating a large number of BCM operations?

In bcm_rx_setup() and bcm_tx_setup(), new operation structures and their
associated frame arrays are allocated unboundedly:

net/can/bcm.c:bcm_rx_setup() {
    ...
    op = kzalloc(OPSIZ, GFP_KERNEL);
    ...
    op->frames = kmalloc_array(msg_head->nframes, op->cfsiz, GFP_KERNEL);
}

Since these allocations do not appear to enforce any per-socket limit
or charge the socket's memory quota (sk_rmem_alloc/sk_wmem_alloc), could an
unprivileged user repeatedly call sendmsg() with unique can_ids to trigger
an out-of-memory panic?

[Severity: Medium]
This is a pre-existing issue, but does bcm_rx_setup() silently fail to
resume frame delivery for updated RX operations if the target interface was
cycled?

When an interface is removed, bcm_notify() handles NETDEV_UNREGISTER by
unregistering the filter and setting op->rx_reg_dev = NULL, but leaves the
op in the active bo->rx_ops list:

net/can/bcm.c:bcm_rx_unreg() {
    ...
    /* mark as removed subscription */
    op->rx_reg_dev = NULL;
}

If an RX_SETUP request subsequently attempts to update this existing
filter, bcm_rx_setup() skips re-registration:

net/can/bcm.c:bcm_rx_setup() {
    ...
    /* Only an update -> do not call can_rx_register() */
    do_rx_register = 0;
}

Should this logic check if the operation is currently unregistered before
clearing do_rx_register?

[ ... ]

> @@ -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 isn't a bug introduced by this patch, but does the handling of
NETDEV_DOWN in bcm_notify() have a data race on bo->ifindex and bo->bound?

While the target patch adds WRITE_ONCE() for bo->bound during device
removal, NETDEV_DOWN appears to read these fields locklessly:

net/can/bcm.c:bcm_notify() {
    ...
    case NETDEV_DOWN:
        if (bo->bound && bo->ifindex == dev->ifindex) {
}

If a socket is connecting to a new device while an unrelated device goes
down, could the lack of barriers allow bcm_notify() to observe a new
bo->bound=1 but the old bo->ifindex, leading to a spurious ENETDOWN error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-bcm_fixes-v9-0-6eebb78ccfb2@hartkopp.net?part=2