Re: [PATCH v15 05/11] can: bcm: add missing rcu list annotations and operations

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional `list_add_rcu()` on the success path in `bcm_rx_setup()` corrupts the list for existing operations.
- [Critical] Removing `list_del_rcu()` from the registration error path in `bcm_rx_setup()` causes a Use-After-Free for existing operations.
--

--- Patch [5]: [PATCH v15 05/11] can: bcm: add missing rcu list annotations and operations ---
commit 493e1bc1997871e18b09125c187a88444720b12c
Author: Oliver Hartkopp <[email protected]>

can: bcm: add missing rcu list annotations and operations

This commit updates the broadcast manager (BCM) to use proper RCU list
operations when adding and removing operations. It addresses missing
list_add_rcu() calls in setup functions and ensures ops are unlinked
before being handed to bcm_remove_op().

Closes: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 03c98e4cc677d..5c1e83eeb4ff3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1449,10 +1450,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					      bcm_rx_handler, op, "bcm", sk);
>  		if (err) {
>  			/* this bcm rx op is broken -> remove it */
> -			list_del_rcu(&op->list);
>  			bcm_remove_op(op);
>  			return err;
>  		}

[Severity: Critical]
Does removing this list_del_rcu() cause a use-after-free for existing
operations?

If a user updates an existing BCM RX operation for a specific device that
was unregistered, the op remains in the list but op->rx_reg_dev is cleared.

If the update occurs and the interface is gone, dev_get_by_index() fails,
and this error path is taken.

Since bcm_remove_op() frees the operation via call_rcu(), removing
list_del_rcu() leaves a freed operation linked in bo->rx_ops. Any
subsequent operation traversing the list, such as bcm_release() or procfs
reads, would then dereference freed memory.

> +
> +		/* add this bcm_op to the list of the rx_ops */
> +		list_add_rcu(&op->list, &bo->rx_ops);
>  	}

[Severity: Critical]
Could this unconditional list_add_rcu() corrupt the list if the operation
is already linked?

When updating an existing operation that needs to be re-registered (for
example, if the interface was unregistered and a new device reuses the
ifindex), do_rx_register will be 1 for an operation that is already linked
in bo->rx_ops.

Calling list_add_rcu() on a node that is already part of the list corrupts
the list pointers, which could lead to an infinite loop or panic during RCU
traversal.

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