Re: [PATCH v5 2/8] 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] Unbounded kernel memory allocation in BCM RX_SETUP and TX_SETUP handlers.
- [High] Missing RCU list primitives causes data races and memory ordering vulnerabilities for `rx_ops` and `tx_ops`.
- [High] Data races and state corruption due to unprotected direct copy into active CAN frame arrays.
- [High] Device unregistration race in `bcm_delete_rx_op` causes netdevice leak and Use-After-Free.
--

--- Patch [2]: [PATCH v5 2/8] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 055258653f72b3386825203a828327e5ab3630d0
Author: Oliver Hartkopp <socketcan@hartkopp.net>

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

This commit fixes a race condition where bcm_sendmsg() locklessly reads
interface indices while other functions mutate them under lock. It moves
these checks under the socket lock to ensure consistency. It also fixes a
silent success return in bcm_rx_setup() when the target device concurrently
disappears.

Link: 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
> 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 does this allocation lack socket memory
limits?

When allocating the operation structure and frame array in bcm_rx_setup()
and bcm_tx_setup():

op = kzalloc(OPSIZ, GFP_KERNEL);
...
op->frames = kmalloc_array(msg_head->nframes, op->cfsiz, GFP_KERNEL);

Could an unprivileged user exhaust kernel memory by creating a CAN BCM socket
and repeatedly sending RX_SETUP or TX_SETUP messages with unique can_id values,
since these allocations are not accounted against sk_omem_alloc or
sk_wmem_alloc?

[Severity: High]
This is a pre-existing issue, but is there a data race when updating frames?

In bcm_rx_setup() and bcm_tx_setup(), incoming frame data is copied directly
into the active op->frames array without holding a lock to protect the buffers:

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

Could the cyclic timeout handlers call bcm_can_tx() and read from op->frames
locklessly while this copy is happening?

This might allow bcm_can_tx() to read and process torn or partially-updated
CAN frames, potentially leading to out-of-bounds reads in drivers if frame
lengths are corrupted.

[ ... ]

> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
[ ... ]
>  	switch (msg_head.opcode) {
>  
>  	case TX_SETUP:

[Severity: High]
This is a pre-existing issue, but do the rx_ops and tx_ops list modifications
need RCU memory barriers?

In bcm_rx_setup() and bcm_tx_setup(), operations are inserted into the lists.
While rx_ops uses list_add_rcu() in some places, tx_ops uses the standard
list_add():

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

Readers such as bcm_proc_show() iterate over these lists locklessly under
rcu_read_lock():

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

Could this cause data races on weakly-ordered architectures where the CPU
reorders the initialization of the new node's pointers to occur after it
becomes visible in the list?

[Severity: High]
This is a pre-existing issue, but does bcm_delete_rx_op() leak device
references?

When userspace removes an RX operation via bcm_delete_rx_op(), it validates
the device by calling dev_get_by_index(). If the device is concurrently
unregistering, it returns NULL:

dev = dev_get_by_index(sock_net(op->sk), op->ifindex);
if (dev) {
        bcm_rx_unreg(dev, op);
        dev_put(dev);
}

If dev is NULL, bcm_rx_unreg() is skipped but the op is freed anyway.

Since the op is removed from bo->rx_ops, a subsequent NETDEV_UNREGISTER
event in bcm_notify() will not find it either.

Does this leave the bcm_rx_handler permanently registered to a freed op
structure?

[ ... ]

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