Re: [PATCH 1/5] 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:
- [High] Deferring timer cancellation to a workqueue fixes a UAF on `bcm_op`, but exposes a Use-After-Free (UAF) on `op->sk` because `bcm_release` does not wait for the workqueue to flush before freeing the socket.
--

commit 9591bb7d40d0a4fbd45c8700cf885c74ad1dcfec
Author: Lee Jones <lee@kernel.org>

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

This commit defers the cancellation of timers and deallocation of bcm_op to
a dedicated workqueue. This prevents a use-after-free when a concurrent
RCU reader re-arms the timer after the RCU callback is scheduled.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..c49b09f3229fa 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -806,6 +813,14 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
>  	kfree(op);
>  }
>  
> +static void bcm_free_op_rcu(struct rcu_head *rcu_head)
> +{
> +	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
> +
> +	INIT_WORK(&op->work, bcm_free_op_work);
> +	queue_work(bcm_wq, &op->work);
> +}

[Severity: High]
Could this deferral to a workqueue expose a use-after-free on op->sk?

If a socket is closed immediately after an operation is deleted:

1. bcm_delete_rx_op() removes op from the socket's rx_ops list and schedules
   call_rcu().
2. A concurrent bcm_rx_handler() running under RCU re-arms op->thrtimer.
3. bcm_release() closes the socket. Since op was removed from rx_ops, it is
   skipped, and bcm_release() drops the final reference via sock_put(sk),
   freeing the socket.

Because the workqueue execution is asynchronous, bcm_release() does not wait
for it. If the re-armed thrtimer fires before bcm_free_op_work() can cancel
it, would this trigger a use-after-free?

bcm_rx_thr_handler()
  bcm_send_to_user()
    reason = sock_queue_rcv_skb_reason(op->sk, skb); 

Since op->sk is assigned in bcm_rx_setup() without a corresponding
sock_hold(), does the deferred cleanup need to hold a socket reference to
ensure the socket outlives this work item and any pending timers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net?part=1