Re: [PATCH v14 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:
- [High] Calling `list_add_rcu()` on an existing `bcm_op` structure causes list corruption when an existing RX operation is re-registered.
- [High] Missing `list_del_rcu()` on the error path causes an existing `bcm_op` to be freed while still in the socket's list.
--

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

can: bcm: add missing rcu list annotations and operations

This commit updates bcm_rx_setup() and bcm_tx_setup() to use list_add_rcu()
when adding operations, ensuring the bcm_op structure is properly initialized
before traversal. It also updates bcm_release() to correctly unlink ops using
list_del_rcu() before freeing them.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 38c3f690c1cd0..940cd9df1cca3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1431,10 +1432,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: High]
Does removing this list_del_rcu() introduce a use-after-free if an existing
operation fails to re-register?

If a virtual CAN interface is unregistered, the netdev notifier sets
op->rx_reg_dev to NULL but keeps the op in bo->rx_ops. If the socket updates
the RX operation while the interface is still gone, bcm_rx_setup() sets
do_rx_register = 1. If dev_get_by_index() then fails and returns an error,
this path is taken.

Since list_del_rcu() was removed, bcm_remove_op() will free the operation
via call_rcu() while it is still linked in bo->rx_ops, which could lead to
a crash during subsequent RCU list traversals.

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

[Severity: High]
Does this unconditionally add the operation to the list, even if it is an
existing operation that is already linked?

Similar to the previous case, if an existing op is being updated and needs to
be re-registered because op->rx_reg_dev was cleared by a netdev unregister
event, do_rx_register is true. 

If the registration succeeds, will this call list_add_rcu() on an op that
is already in bo->rx_ops, overwriting its list pointers and corrupting the
list?

>  	}
>  
>  	return msg_head->nframes * op->cfsiz + MHSIZ;

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