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

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

Pre-existing issues:
- [Critical] In-place modification of `op->frames` via `memcpy_from_msg()` causes data races and heap Out-Of-Bounds reads
- [High] `bcm_rx_setup` and `bcm_tx_setup` add elements to RCU-protected lists using `list_add` instead of `list_add_rcu`
- [High] `bcm_proc_show` iterates over the RCU-protected list `bo->tx_ops` using `list_for_each_entry` instead of `list_for_each_entry_rcu`
--

--- Patch [1]: [PATCH v12 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 49ab02a8012b5c460ed2235ffc2a8c72c64c1653
Author: Lee Jones <[email protected]>
Subject: can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op()
with call_rcu() and introduced the RX_NO_AUTOTIMER flag.

However, this flag check was omitted for thrtimer in the packet rx
fast-path. During BCM RX operation teardown, a concurrent RCU reader
can race and re-arm thrtimer via bcm_rx_update_and_send() after
call_rcu() has been scheduled. Once the RCU grace period elapses,
bcm_op is freed and the subsequently firing thrtimer dereferences
the deallocated op.

> 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;
>  
>  		/* initialize uninitialized (kzalloc) structure */

[Severity: Critical]
This is a pre-existing issue, but does bcm_tx_setup() allow invalid frames
to be sent?

If a user creates an active TX operation with a timer, and then updates it
by calling sendmsg() with a frame length greater than 64:

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

Because memcpy_from_msg() modifies op->frames before validation,
the corrupted frame remains on error and bcm_tx_setup() returns -EINVAL.

However, the timer continues running and transmits the corrupted frame
via bcm_tx_timeout_handler().  Since can_send() and drivers blindly trust
cf->len, could CAN drivers read past the 72-byte SKB buffer into adjacent
heap memory?

[Severity: High]
This isn't a bug introduced by this patch, but are we using the correct
list insertion function here?

bcm_tx_setup() inserts new elements into bo->tx_ops using list_add()
instead of list_add_rcu(). Because list_add() lacks smp_store_release()
barriers, can a concurrent RCU reader like bcm_proc_show() read the list
pointer before the CPU writes the kzalloc initialization, leading to a
wild pointer dereference?

> @@ -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: Critical]
This is a pre-existing issue, but does bcm_rx_setup() suffer from a similar
data race as bcm_tx_setup()?

bcm_rx_setup() also modifies op->frames directly:

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

Can this be modified concurrently with bcm_rx_handler() without
synchronization?

[Severity: High]
This isn't a bug introduced by this patch, but does bcm_rx_setup() also insert
new elements into bo->rx_ops using list_add() without RCU barriers,
similar to bcm_tx_setup()?

[Severity: High]
This is a pre-existing issue, but does bcm_proc_show() safely traverse
the RCU list?

bcm_proc_show() iterates over the RCU-protected list bo->tx_ops:

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

Since list_for_each_entry() lacks the READ_ONCE() wrapper for fetching
pos->next, if bcm_delete_tx_op() deletes an element concurrently, could the
compiler reload the list pointer unpredictably and violate RCU safety
guarantees?

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