Re: [PATCH v15 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure

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

Pre-existing issues:
- [High] Missing RCU synchronization primitives when publishing to or traversing the rx_ops and tx_ops lists, leading to potential data corruption or kernel oops for concurrent procfs readers.
--

--- Patch [2]: [PATCH v15 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit ebe4ed63d28eb9026b5f264dfe90ce6952b3275f
Author: Oliver Hartkopp <[email protected]>

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

This commit fixes a lockless bound/ifindex race by moving the ifindex read
and bound re-check into the locked section. It also ensures bcm_rx_setup()
returns -ENODEV 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;
>  			}
>  
>  		} else

[Severity: High]
This is a pre-existing issue, but is it possible for a concurrent procfs
reader to observe uninitialized list nodes due to missing memory barriers?

In net/can/bcm.c:bcm_tx_setup(), list_add() is used instead of
list_add_rcu() when adding to tx_ops:

    /* add this bcm_op to the list of the tx_ops */
    list_add(&op->list, &bo->tx_ops);

Similarly, in net/can/bcm.c:bcm_rx_setup(), list_add() is used instead of
list_add_rcu() when adding to rx_ops. This misses the memory barriers needed
to ensure node initialization is visible to concurrent RCU readers.

Additionally, in net/can/bcm.c:bcm_proc_show(), list_for_each_entry() is
used instead of list_for_each_entry_rcu() to traverse tx_ops inside an RCU
critical section:

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

This misses READ_ONCE() and data dependency barriers.

Could an unprivileged userspace program repeatedly adding or removing TX/RX
operations using bcm_sendmsg() (while holding lock_sock()) cause a
concurrent thread reading the corresponding /proc/net/can-bcm/<inode> file
(holding only rcu_read_lock()) to encounter an information leak or a kernel
oops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2