From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0D540313293 for ; Sun, 12 Jul 2026 12:54:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783860858; cv=none; b=RyqrTU8UImt1x+X5CJPx8OeLTSh19QWRVVbn9kEFv3z+2KSBqVCnfweOBfTzOQJ7XT3JwF49IDWDkWKfRmwiU8hYr4Yyn5zvMxpS2LLIAYjBkjcToWrr9khOaiT+auJGj1XoB+etQaFWyPF0K7N/vQJ58ZU0fRsa8JnH9yMnz1A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783860858; c=relaxed/simple; bh=JD5ce/UgtVO3cliiB9aqKhwRTyRV4GqtxiDecSUZzwE=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=GDI6zwjcP2MuWo5l0WNhIyO/2IKBUtzpTjWWYDJyFlAHa/cWHgmbPHhhgWZ9p7tjVJC/CnwtZknmP22zv3PVFdbo3HXfJrJjLewUs2sNWGqRU38nLPfSCvZlGzomXxjglMKP8+yJzxixRaHAasHKygoI9VGIyMOY1CtrHU15kJM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cVXPc0Wm; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cVXPc0Wm" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5183B1F000E9; Sun, 12 Jul 2026 12:54:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783860856; bh=mvGDy2zOsEy155hJ7c08IH6Sgw3gAMIq+RFB/RL2SLY=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=cVXPc0WmLI9fRg+QnXi6YvekQ0N9iTBp0QDqlJ7jA7s2HthUPWRcPbNJHpR/CweJd caYrMOPvVoX93iQead1hiRUP60VjMIgog4fC2sRbAMAqWcuHrMF7R+mpkHpg8ypxsL Gjz4G1Ia+s+ZOEBaiEN/9EJklfViaVrOXjwR9iiVv0tsXTlE8onIs2NOQI6gyVCO/j kSU1fbkc8j1UWTC5J06zYEFwYss1IcL7Djj7hwJ7pn3QBYSUQjGDdRj1Pn3+wbAxYB moYCwKvjYv5XNKR8LFCZKKborRpL3BhKJT2Cl3lwdrTKpXKidxIbPZ9ukxFmhDJixa S7MP+2IjtjVXQ== From: sashiko-bot@kernel.org Subject: Re: [PATCH v8 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release() Reply-To: sashiko-reviews@lists.linux.dev To: "Oliver Hartkopp" Cc: "Vincent Mailhol" , "Marc Kleine-Budde" , linux-can@vger.kernel.org, "Oliver Hartkopp" , "Oleksij Rempel" In-Reply-To: <20260712-isotp-fixes-v8-1-c39220a4e442@hartkopp.net> References: <20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net> <20260712-isotp-fixes-v8-1-c39220a4e442@hartkopp.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sun, 12 Jul 2026 12:54:15 +0000 Message-Id: <20260712125416.5183B1F000E9@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-can@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 5 potential issue(= s) to consider: Pre-existing issues: - [Critical] Race condition between isotp_sendmsg() error paths and isotp_r= cv_fc() allows leaking uninitialized kernel heap memory over the CAN bus. - [High] CAN filter leak due to a race condition between socket release and= netdevice unregistration. - [High] State Machine Leak and Cross-Interface Data Leakage on NETDEV_UNRE= GISTER. - [High] Lockless updates to the TX state machine in isotp_sendmsg() race w= ith concurrent CAN frame reception. - [Medium] Missing Error Handling in isotp_bind() ignores can_rx_register()= failures. -- --- Patch [1]: [PATCH v8 1/4] can: isotp: use unconditional synchronize_rcu= () in isotp_release() --- commit 6e5426def22a6e6426722ba05c1e7413dd0ccd86 Author: Oliver Hartkopp can: isotp: use unconditional synchronize_rcu() in isotp_release() isotp_notify() unregisters the (RCU) CAN filters and clears so->bound without waiting for a grace period. This commit updates isotp_release() to unconditionally call synchronize_rcu() before cancelling timers, fixing a race condition where NETDEV_UNREGISTER could lead to a use-after-free timer callback on the freed socket. Link: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.ke= rnel.org/ > diff --git a/net/can/isotp.c b/net/can/isotp.c > index c48b4a818297e..d30937345bcdc 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock) > SINGLE_MASK(so->txid), > isotp_rcv_echo, sk); > dev_put(dev); > - synchronize_rcu(); > } > } > } [Severity: High] This is a pre-existing issue, but does a race condition exist between socket release and netdevice unregistration that can leak filters? If isotp_release() runs during the window where unregister_netdevice_many() has unlisted the device from the IDR but before notifiers are invoked: net/can/isotp.c:isotp_release() { ... list_del(&so->notifier); ... dev =3D dev_get_by_index(net, so->ifindex); ... } Because the socket is unlinked from the notifier list, the notifier won't clean it up. However, dev_get_by_index() returns NULL since the device is unlisted, causing isotp_release() to silently skip calling can_rx_unregister() and permanently leak the receiver memory. > + /* Always wait for a grace period before touching the timers below. > + * A concurrent NETDEV_UNREGISTER may have already unregistered our > + * filters and cleared so->bound in isotp_notify() without waiting > + * for in-flight isotp_rcv() callers to finish, so this call must not > + * be skipped just because so->bound is already 0 here. > + */ > + synchronize_rcu(); > + > hrtimer_cancel(&so->txfrtimer); > hrtimer_cancel(&so->txtimer); > hrtimer_cancel(&so->rxtimer); [Severity: High] This isn't a bug introduced by this patch, but does the NETDEV_UNREGISTER handler fail to stop active state machines? When the unregister event is handled: net/can/isotp.c:isotp_notify() { ... case NETDEV_UNREGISTER: lock_sock(sk); /* remove current filters & unregister */ ... } The code clears so->bound and so->ifindex but omits explicitly aborting active state machines (so->tx.state) or cancelling tx timers. If a thread is blocked in isotp_sendmsg(), it remains hung until a timer expires. Additionally, if userspace calls bind() on a new interface, the state isn't reset, and when the active timer fires, it could transmit remaining frames of the old PDU out of the newly bound network interface. [Severity: Medium] This is a pre-existing issue, but does isotp_bind() ignore filter registration failures? net/can/isotp.c:isotp_bind() { ... if (isotp_register_rxid(so)) can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), isotp_rcv, sk, "isotp", sk); ... } If can_rx_register() fails with -ENOMEM, isotp_bind() ignores the return value, sets so->bound =3D 1, and returns 0. The socket then silently fails = to receive or echo frames, and later isotp_release() will trigger a warning splat when attempting to unregister the missing filter. [Severity: High] This isn't a bug introduced by this patch, but is there a race with concurrent CAN frame reception when updating the state locklessly? net/can/isotp.c:isotp_sendmsg() { ... so->tx.state =3D ISOTP_WAIT_FIRST_FC; ... } If a spoofed or leftover Flow Control frame is received concurrently on another CPU: net/can/isotp.c:isotp_rcv_fc() { ... if (so->tx.state !=3D ISOTP_WAIT_FC && so->tx.state !=3D ISOTP_WAIT_FIRST_FC) ... } The softirq path can see ISOTP_WAIT_FIRST_FC and immediately transition to ISOTP_SENDING, transmitting a Consecutive Frame before isotp_sendmsg() has sent the First Frame, scrambling the protocol state. [Severity: Critical] This is a pre-existing issue, but can the signal error path in isotp_sendmsg() leak uninitialized kernel memory? If isotp_sendmsg() is interrupted by a signal, it sets the state to idle: net/can/isotp.c:isotp_sendmsg() { ... so->tx.state =3D ISOTP_IDLE; hrtimer_cancel(&so->txfrtimer); ... } If a concurrent isotp_rcv_fc() already passed the state check, and userspace immediately calls sendmsg() again with a large size: net/can/isotp.c:isotp_sendmsg() { ... u8 *newbuf =3D kmalloc(max_pdu_size, GFP_KERNEL); ... } The delayed isotp_rcv_fc() could resume, transition the state, and transmit the newly allocated, uninitialized so->tx.buf over the CAN bus before the data has been copied from userspace. --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260712-isotp-fixe= s-v8-0-c39220a4e442@hartkopp.net?part=3D1