Re: [PATCH] can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race allowing transmission of unvalidated CAN frames with out-of-bounds lengths.
- [Medium] Missing RCU list traversal primitive for tx_ops in bcm_proc_show().
- [Medium] Unrecoverable state corruption upon validation failure during BCM operation update.
--

commit 6728a4ac4d52665b9a664d80e0c10b8ebce20e70
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: use list_add_rcu() in bcm_[rx|tx]_setup()

This patch moves the list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() to the end of the initialization code. This ensures the
bcm_op structure is fully initialized before it becomes visible to RCU
readers like bcm_proc_show().

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 a4bef2c48a559..7215fa32832c8 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -930,6 +930,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: High]
This is a pre-existing issue, but is it possible for an unvalidated
CAN frame to be transmitted here?

When updating an existing operation in bcm_tx_setup(), the user data is
copied directly into the active transmission buffer before validation:

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

Concurrently, the softirq timer bcm_tx_timeout_handler() could fire,
calling bcm_can_tx(), which fetches this frame and sends it without locks:

    skb_put_data(skb, cf, op->cfsiz);
    err = can_send(skb, 1);

Could a malicious user craft a frame with an out-of-bounds length
(e.g., cf->len = 255) that gets fetched and transmitted before
bcm_tx_setup() validates it and aborts? This could potentially cause
crashes or out-of-bounds memory reads in downstream CAN drivers.


[Severity: Medium]
This is a pre-existing issue, but does a failed validation leave the socket
in an unrecoverable corrupted state?

In both bcm_tx_setup() and bcm_rx_setup(), if an existing operation is being
updated and the validation fails after the memcpy_from_msg() block:

    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    if (err < 0)
        return err;

The function returns an error without reverting the changes to op->frames.
Because the timers might still be running for the active bcm_op, the socket
seems permanently stuck transmitting or filtering based on this partially
overwritten data.

[ ... ]
> @@ -1093,6 +1093,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		op->flags |= TX_ANNOUNCE;
>  	}
>  
> +	/* add this bcm_op to the list of the tx_ops? */
> +	if (add_op_to_list)
> +		list_add_rcu(&op->list, &bo->tx_ops);
> +

[Severity: Medium]
This is a pre-existing issue, but since we are now using list_add_rcu()
here, should the iteration in bcm_proc_show() also use the RCU primitive?

bcm_proc_show() traverses the bo->tx_ops list under rcu_read_lock(), but
currently uses the standard list macro:

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

Without list_for_each_entry_rcu(), it omits the necessary rcu_dereference()
calls and memory barriers. On weakly ordered architectures, couldn't this
lead to reading torn pointers or uninitialized memory of a newly inserted
bcm_op, potentially causing a crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610163744.51980-2-socketcan@hartkopp.net?part=1