Re: [PATCH v8 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 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Data race and potential memory corruption when updating an existing RX operation's filter frames.
- [High] Unsafe lockless iteration over `bo->tx_ops` in procfs handler.
- [High] TOCTOU data race allowing transmission of unvalidated cf->len, leading to Information Leak.
- [High] Missing validation of cf->len for RX_SETUP messages allows OOB read on RTR replies.
- [High] Unbounded kernel memory allocation in TX_SETUP and RX_SETUP enables local DoS.
--

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

This patch defers the teardown and memory deallocation of bcm_op structures
to a dedicated workqueue. This prevents use-after-free issues caused by
timers firing after RCU grace periods, and avoids sleeping in softirq
context. It also adds socket reference counting during the deferred work.

> 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: High]
This is a pre-existing issue, but is there an unsafe lockless iteration
over bo->tx_ops in the procfs handler?

In bcm_tx_setup(), elements are added to the bo->tx_ops list using
list_add(), which lacks the memory barriers of list_add_rcu(). However,
bcm_proc_show() iterates this list using list_for_each_entry() while only
holding rcu_read_lock().

If a user continuously creates TX operations while concurrently reading the
socket's procfs file, could the procfs reader observe an uninitialized next
pointer and cause a crash due to an invalid pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does bcm_tx_setup() contain
a time-of-check to time-of-use (TOCTOU) data race?

If a user updates an existing TX operation while its cyclic timer is firing,
bcm_tx_setup() directly modifies op->frames with user-supplied data before
validating it:

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

If the timer concurrently triggers bcm_tx_timeout_handler(), which calls
bcm_can_tx(), it can copy the partially updated frame into an SKB. Could
the CAN driver then read past the SKB data bounds based on an unvalidated
cf->len, leading to an out-of-bounds read and information leak?

[Severity: High]
This isn't a bug introduced by this patch, but do the allocations in
bcm_tx_setup() and bcm_rx_setup() lack memory limits?

A local user can open a CAN_BCM socket (which doesn't require
CAP_NET_ADMIN) and loop sendmsg() with TX_SETUP or RX_SETUP opcodes
using unique CAN IDs.

Since these functions allocate large structures and frame arrays using
kzalloc() and kmalloc_array() without charging the memory against the
socket's limits (for example, via sk_mem_charge()), could an attacker
exhaust kernel memory and cause a local denial of service?

[ ... ]

> @@ -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;
>  
>  		/* ifindex for timeout events w/o previous frame reception */

[Severity: High]
This isn't a bug introduced by this patch, but there is a data race when
updating an existing RX operation's filter frames.

When a user updates an existing RX filter via bcm_sendmsg(), bcm_rx_setup()
copies data directly into op->frames using memcpy_from_msg() and clears
op->last_frames via memset() without holding a lock.

Concurrently, can a packet reception trigger bcm_rx_handler() in softirq
context, causing it to read these arrays locklessly and observe torn or
zeroed state?

[Severity: High]
This is a pre-existing issue, but it looks like bcm_rx_setup() is missing
length validation for the supplied RX configuration frames.

When a user configures an RX operation with the RX_RTR_FRAME flag and
supplies a frame with a large cf->len, bcm_rx_setup() copies it via
memcpy_from_msg() but fails to validate the length.

When a matching RTR request is received, bcm_rx_handler() invokes
bcm_can_tx() to reply. Could the CAN driver then blindly trust cf->len
and read out of bounds on the SKB buffer, exposing adjacent kernel heap
memory on the CAN bus?

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