Re: [PATCH net 10/19] can: bcm: add missing rcu list annotations and operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] List corruption and Use-After-Free in bcm_rx_setup() due to unconditional list_add_rcu() and missing list_del_rcu() on existing operations.
--

--- Patch [10]: [PATCH net 10/19] can: bcm: add missing rcu list annotations and operations ---
commit 920bd90c8dec1b6179ef18a6d82400e3e89b0659
Author: Oliver Hartkopp <[email protected]>

can: bcm: add missing rcu list annotations and operations

This commit aims to fix RCU list annotations for CAN BCM sockets so that
bcm_proc_show() can safely traverse the operation lists. It moves the
list_add_rcu() calls to the end of the bcm_tx_setup() and bcm_rx_setup()
routines to ensure structures are fully initialized when published. It also
adds list_del_rcu() before freeing operations in bcm_release().

Link: https://patch.msgid.link/[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;
>  		}
> +
> +		/* add this bcm_op to the list of the rx_ops */
> +		list_add_rcu(&op->list, &bo->rx_ops);
>  	}
>  
>  	return msg_head->nframes * op->cfsiz + MHSIZ;

[Severity: Critical]
If an existing operation is updated after an interface bounce, could this
cause list corruption or a use-after-free?

When a CAN interface goes down, bcm_notify() unregisters the receive
operation but intentionally leaves the bcm_op linked in bo->rx_ops. If
userspace later updates this existing operation via a new RX_SETUP message,
bcm_rx_setup() finds the existing operation and sets do_rx_register = 1.

If the subsequent re-registration fails (err != 0), it looks like removing
list_del_rcu() here means bcm_remove_op() will free the operation via
call_rcu() while it is still linked in the bo->rx_ops list.

Conversely, if re-registration succeeds, won't the unconditional call to
list_add_rcu() corrupt the list pointers since the operation is already
linked in the list?

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