Re: [PATCH net 06/19] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

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

Pre-existing issues:
- [High] Unprotected concurrent modification of `op->frames` in `bcm_tx_setup` can lead to corrupted or torn CAN frames being transmitted.
- [High] Unprotected modification of `op->frames` and `op->flags` in `bcm_rx_setup` races with `bcm_rx_handler` in softirq context.
- [High] The `tx_ops` and `rx_ops` lists are updated without RCU list primitives, creating a data race with concurrent readers in `bcm_proc_show()`.
- [Medium] Concurrent execution of `bcm_rx_handler` on multiple CPUs causes data corruption for `op->last_frames` and statistics.
--

--- Patch [6]: [PATCH net 06/19] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 542011e18159cf5ca68d911f15d0136be9c07cb9
Author: Lee Jones <[email protected]>
Subject: can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

This commit resolves a use-after-free by deferring timer cancellation and 
memory deallocation to a dedicated unbound workqueue. It also takes a 
reference on op->sk to ensure the socket is not freed while timers are 
still armed.

Link: https://patch.msgid.link/[email protected]

> 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);
>  		op->ifindex = ifindex;

[Severity: High]
This isn't a bug introduced by this patch, but does the unprotected 
modification of op->frames in bcm_tx_setup() allow torn CAN frames to be 
transmitted?

When updating an existing operation, the code copies data from userspace:

bcm_tx_setup() {
    ...
    err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
    ...
}

Could a concurrent bcm_can_tx() executed by the TX timer read from 
op->frames while the copy is happening, since the timer drops the 
bcm_tx_lock before calling skb_put_data(skb, cf, op->cfsiz)?

[Severity: High]
This is a pre-existing issue, but does adding new elements to the ops 
lists create a data race with concurrent readers?

When adding a new element, the setup functions call list_add():

bcm_tx_setup() {
    ...
    list_add(&op->list, &bo->tx_ops);
    ...
}

Since list_add() lacks RCU write barriers, could concurrent readers in 
bcm_proc_show() observe torn list pointers when iterating over tx_ops 
under rcu_read_lock()?

[ ... ]
> @@ -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);
>  		op->ifindex = ifindex;

[Severity: High]
This is a pre-existing issue, but can bcm_rx_setup() race with 
bcm_rx_handler() in softirq context when modifying op->frames?

When bcm_rx_setup() updates an existing operation, it copies frame data:

bcm_rx_setup() {
    ...
    err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
    ...
}

Since this is only protected by lock_sock() and not serialized against 
softirq, could an incoming CAN frame trigger bcm_rx_handler() 
concurrently, leading to inconsistent filter configurations and 
out-of-bounds array accesses when op->nframes changes?

[Severity: Medium]
This isn't a bug introduced by this patch, but are the shared RX 
statistics and op->last_frames vulnerable to data corruption?

If bcm_rx_handler() runs concurrently on multiple CPUs for the same 
bcm_op, it performs non-atomic updates:

bcm_rx_handler() {
    ...
    op->frames_abs++;
    ...
    bcm_rx_update_and_send(op, op->last_frames, rxframe, traffic_flags);
    ...
}

Does modifying op->last_frames and op->frames_abs without holding a 
spinlock allow torn writes and lost increments?

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