Re: [PATCH v7 01/10] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent lockless access to op->frames and metadata between process context setups (bcm_rx_setup / bcm_tx_setup) and softirq handlers (bcm_rx_handler / bcm_can_tx).
--

--- Patch [1]: [PATCH v7 01/10] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit a3f554e14b7943cd3930531fdd91052fab6a02ca
Author: Lee Jones <lee@kernel.org>

can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

This commit resolves a use-after-free issue when tearing down BCM RX
operations by deferring the cancellation of timers and memory deallocation
to a dedicated workqueue. It also takes a reference on the socket to ensure
it is not freed while timers are still armed.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..bdf53241bd7b3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1060,6 +1081,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  		/* bcm_can_tx / bcm_tx_timeout_handler needs this */
>  		op->sk = sk;
> +		sock_hold(sk);

[Severity: High]
This is a pre-existing issue, but does bcm_tx_setup() overwrite op->frames
directly from userspace without holding bcm_tx_lock?

When updating an existing operation, bcm_tx_setup() appears to do:

    /* update CAN frames content */
    for (i = 0; i < msg_head->nframes; i++) {
        cf = op->frames + op->cfsiz * i;
        err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);

While concurrently, bcm_can_tx() drops the lock before dereferencing a
pointer into op->frames:

    /* read currframe under lock protection */
    spin_lock_bh(&op->bcm_tx_lock);
    cf = op->frames + op->cfsiz * op->currframe;
    spin_unlock_bh(&op->bcm_tx_lock);
    ...
    skb_put_data(skb, cf, op->cfsiz);

Can this allow a concurrent bcm_tx_setup() on another CPU to overwrite the
frames, leading to the transmission of partially updated (torn) CAN frames?

>  		op->ifindex = ifindex;
>  
>  		/* initialize uninitialized (kzalloc) structure */
[ ... ]
> @@ -1221,6 +1243,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  		/* bcm_can_tx / bcm_tx_timeout_handler needs this */
>  		op->sk = sk;
> +		sock_hold(sk);

[Severity: High]
This is also a pre-existing issue, but does bcm_rx_setup() modify op->frames
locklessly here as well?

    /* update CAN frames content */
    err = memcpy_from_msg(op->frames, msg,
                          msg_head->nframes * op->cfsiz);

This bypasses bcm_rx_update_lock. In bcm_rx_handler(), it attempts to read
safely by taking the lock:

    /* snapshot RTR content under lock: op->flags/op->frames may be
     * updated concurrently by bcm_rx_setup().
     */
    spin_lock_bh(&op->bcm_rx_update_lock);
    ...
    rtr_frame = op->flags & RX_RTR_FRAME;
    if (rtr_frame)
        memcpy(&rtrframe, op->frames, op->cfsiz);

Because bcm_rx_setup() does not acquire bcm_rx_update_lock when modifying
op->frames directly from userspace, can this result in bcm_rx_handler()
acting on inconsistent metadata or evaluating torn multiplex masks?

>  		op->ifindex = ifindex;
>  
>  		/* ifindex for timeout events w/o previous frame reception */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v7-0-c78896b342e4@hartkopp.net?part=1