Re: [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] bcm_tx_setup() updates CAN frames directly in the active op->frames buffer without locking and before validation, allowing a concurrent timer to transmit unvalidated, malformed frames.
- [Medium] bcm_tx_setup() updates 64-bit ktime_t timer variables without locking, risking torn reads on 32-bit systems.
- [Medium] Memory leak of CAN receiver object due to race between bcm_delete_rx_op() and NETDEV_UNREGISTER.
--

--- Patch [5]: [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations ---
commit f371f28179652ae60b7b0cd6b25f7f33429dc82a
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add missing rcu list annotations and operations

This patch fixes missing list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() by moving them to the end of the setup code. It also fixes
bcm_release() to properly call list_del_rcu() before bcm_remove_op() when
removing operations.

Link: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 340b7fc89744e..fca7c714cc52f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -999,6 +999,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  	struct bcm_sock *bo = bcm_sk(sk);
>  	struct bcm_op *op;
>  	struct canfd_frame *cf;
> +	bool add_op_to_list = false;
>  	unsigned int i;
>  	int err;

[Severity: Critical]
This is a pre-existing issue, but does bcm_tx_setup() update CAN frames
directly in the active op->frames buffer without locking and before
validation?

When updating an existing TX operation, user data is copied directly into
op->frames without holding bcm_tx_lock:

	err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
	if (op->flags & CAN_FD_FRAME) {
		if (cf->len > 64)
			err = -EINVAL;

Since the operation is already active, a concurrent bcm_tx_timeout_handler()
could trigger and call bcm_can_tx(). The timer handler acquires bcm_tx_lock,
copies the torn/unvalidated frame, and transmits it:

	memcpy(&cframe, op->frames + op->cfsiz * op->currframe, op->cfsiz);
	cf = &cframe;

If an unprivileged user provides an invalid length, could
bcm_tx_timeout_handler() transmit it before bcm_tx_setup() rejects it,
leading to an out-of-bounds read of the kernel skb?

[Severity: Medium]
This is also a pre-existing issue, but are the 64-bit ktime_t timer
variables updated without lock protection in bcm_tx_setup()?

When the SETTIMER flag is provided, the variables are updated directly:

	op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
	op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);

The timer handler reads these ktime_t values locklessly in
bcm_tx_set_expiry():

	if (op->kt_ival1 && op->count)
		ival = op->kt_ival1;
	else if (op->kt_ival2)
		ival = op->kt_ival2;

On 32-bit systems, could this result in a torn read of the 64-bit ktime_t
values, causing bogus timer expiration times?

[ ... ]
> @@ -1790,8 +1795,10 @@ static int bcm_release(struct socket *sock)
>  
>  	synchronize_rcu();
>  
> -	list_for_each_entry_safe(op, next, &bo->rx_ops, list)
> +	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
> +		list_del_rcu(&op->list);
>  		bcm_remove_op(op);
> +	}
>  
>  	/* remove device reference */
>  	if (bo->bound) {

[Severity: Medium]
This is a pre-existing issue, but is there a memory leak of the CAN
receiver object due to a race between bcm_delete_rx_op() and
NETDEV_UNREGISTER?

If bcm_delete_rx_op() races with unregister_netdevice(), dev_get_by_index()
returns NULL:

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

The op is then removed from the list via list_del_rcu(&op->list):

	list_del_rcu(&op->list);
	bcm_remove_op(op);

Making it invisible to the subsequent NETDEV_UNREGISTER event handler in
bcm_notifier(). Since bcm_rx_unreg() is skipped in both places, does this
permanently leak the receiver object allocated in can_rx_register()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=5