[PATCH net 0/14] pull-request: can 2026-08-26

[PATCH net 0/14] pull-request: can 2026-08-26

[Marc Kleine-Budde]

Hello netdev-team,

this is a pull request of 14 patches for net-next/main.

The first patch is by Vincent Mailhol to drop CAN-XL frames on non
CAN-XL devices.

Oliver Hartkopp's patch updates the CAN networking core to use a more
robust check to ensure working in CAN devices.

Sureshkumar S cotributes 2 patches to fix corner cases of the the CAN
bittiming calculation.

The next 3 patches are by Cunhao Lu and fix the TX path of the
rockchip_canfd driver.

Another 3 patches by Cunhao Lu make echo skb freeing safe in any IRQ
context in several CAN helper functions.

Fan Wu's for the kvaser_pciefd driver fixes a potential
use-after-free.

Cen Zhang fixes a slab-out-of-bounds read access in the kvaser_usb
driver.

Ji-Ze Hong fixes a struct size mismatch in the f81604 CAN driver.

The last patch is by Runyu Xiao and fixes a potential deadlock in the
error of the hi311x CAN driver.

regards,
Marc

---

The following changes since commit dc4b95b8fee95113587e93ca116356032d271371:

  net/sched: sch_teql: restore skb->dev on the slave failure path (2026-08-25 15:24:35 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-7.3-20260826

for you to fetch changes up to 475cb4ea138f4b0e7f43ff79220f4013a6a840b8:

  can: hi311x: drop hi3110_lock before free_irq() on open failure (2026-08-26 13:40:17 +0200)

----------------------------------------------------------------
linux-can-fixes-for-7.3-20260826

----------------------------------------------------------------
Cen Zhang (Microsoft) (1):
      can: kvaser_usb: validate command format before parsing in hydra receive path

Cunhao Lu (6):
      can: rockchip_canfd: prevent TX stall on echo skb failure
      can: rockchip_canfd: retry the outstanding TX buffer
      can: rockchip_canfd: serialize TX state and command writes
      can: skb: make echo skb freeing safe in any IRQ context
      can: skb: make CAN skb allocation failure paths IRQ-safe
      can: dev: can_put_echo_skb(): free skb on invalid echo index

Fan Wu (1):
      can: kvaser_pciefd: fix use-after-free in bec poll timer

Ji-Ze Hong (Peter Hong) (1):
      can: usb: f81604: fix struct f81604_int_data size mismatch

Marc Kleine-Budde (3):
      Merge patch series "can: bittiming: fix two defects in the userspace bitrate conversion"
      Merge patch series "can: rockchip_canfd: prevent TX stall on echo skb failure"
      Merge patch series "can: skb: make echo skb freeing safe in any IRQ context"

Oliver Hartkopp (1):
      can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

Runyu Xiao (1):
      can: hi311x: drop hi3110_lock before free_irq() on open failure

Sureshkumar S (2):
      can: bittiming: fix divide-by-zero in can_calc_bittiming()
      can: bittiming: fix bitrate error calculation on unsigned operands

Vincent Mailhol (1):
      can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

 drivers/net/can/dev/calc_bittiming.c               | 10 ++--
 drivers/net/can/dev/skb.c                          | 14 ++---
 drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c |  3 +-
 drivers/net/can/rockchip/rockchip_canfd-core.c     |  1 +
 drivers/net/can/rockchip/rockchip_canfd-rx.c       | 31 ++++++++---
 drivers/net/can/rockchip/rockchip_canfd-tx.c       | 47 ++++++++++++----
 drivers/net/can/rockchip/rockchip_canfd.h          |  4 +-
 drivers/net/can/spi/hi311x.c                       |  3 ++
 drivers/net/can/usb/f81604.c                       |  2 +-
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c  | 62 +++++++++++++++++++---
 include/linux/can/skb.h                            |  4 +-
 net/can/af_can.c                                   | 12 ++---
 net/can/bcm.c                                      |  7 +--
 net/can/gw.c                                       |  7 +--
 net/can/isotp.c                                    |  5 +-
 net/can/raw.c                                      |  4 +-
 16 files changed, 160 insertions(+), 56 deletions(-)

[PATCH net 01/14] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[Marc Kleine-Budde]

From: Vincent Mailhol <mailhol@kernel.org>

Sending a PF_PACKET bypasses the CAN framework logic and can directly
reach a CAN driver's xmit() function. The PF_PACKET framework only
checks that skb->len does not exceed the net_device MTU.

For a CAN device that is not CAN XL capable, anything above CANFD_MTU
(72 bytes) is therefore dropped before it reaches the driver. However,
CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in
the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to
2060 bytes.

As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes
can pass both the MTU and the can_dropped_invalid_skb() checks.

A driver that does not support CAN XL will interpret canxl_frame->flags
as a length because of the overlap with can_frame->len. And because
CANXL_XLF is set, the resulting length is between 128 and 255. For
drivers that do not check can_frame->len before copying can_frame->data,
as most drivers do, this results in a buffer overflow of up to 247
bytes.

Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL
capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs.

Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1
Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20260731-drop_canxl_frames-v1-1-7387b70353b3@kernel.org
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..4f7a189de265 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
  */
 
+#include <linux/can/can-ml.h>
 #include <linux/can/dev.h>
 #include <linux/module.h>
 #include <net/can.h>
@@ -384,7 +385,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 		break;
 
 	case ETH_P_CANXL:
-		if (!can_is_canxl_skb(skb))
+		if (!can_cap_enabled(dev, CAN_CAP_XL) || !can_is_canxl_skb(skb))
 			goto inval_skb;
 		break;
 

base-commit: dc4b95b8fee95113587e93ca116356032d271371
-- 
2.53.0

[PATCH net 02/14] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct
net_device") introduced an explicit way to assign the midlayer private
pointer (dev->ml_priv) to named users like ML_PRIV_CAN.

With this extension the CAN device specific ml_priv assignment became a
robust indicator to identify a valid CAN device, when can_get_ml_priv()
returns a valid pointer.

This has been used directly by the referenced commit in the CAN specific
j1939 and proc code but not in the other parts of the CAN subsystem.

With the TUN/TAP driver a device's ARPHRD type can be controlled by
userspace independently of its midlayer private data (ml_priv). The
TUNSETLINK ioctl allows a down TUN/TAP device to overwrite its hardware
type to become ARPHRD_CAN while dev->ml_priv remains NULL (uninitialized).

Instead of checking dev->type being the unreliable ARPHRD_CAN value convert
the missing "valid CAN devices" checks to can_get_ml_priv().

Fixes: 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")
Cc: stable@kernel.org
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260819160822.8256-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 12 ++++++------
 net/can/bcm.c    |  7 ++++---
 net/can/gw.c     |  7 ++++---
 net/can/isotp.c  |  5 +++--
 net/can/raw.c    |  4 ++--
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 7bc86b176b4d..ef435f22ac93 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -226,7 +226,7 @@ int can_send(struct sk_buff *skb, int loop)
 		goto inval_skb;
 	}
 
-	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+	if (unlikely(!can_get_ml_priv(skb->dev))) {
 		err = -EPERM;
 		goto inval_skb;
 	}
@@ -452,7 +452,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	/* insert new receiver  (dev,canid,mask) -> (func,data) */
 
-	if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
+	if (dev && !can_get_ml_priv(dev))
 		return -ENODEV;
 
 	if (dev && !net_eq(net, dev_net(dev)))
@@ -519,7 +519,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
-	if (dev && dev->type != ARPHRD_CAN)
+	if (dev && !can_get_ml_priv(dev))
 		return;
 
 	if (dev && !net_eq(net, dev_net(dev)))
@@ -687,7 +687,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_can_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
@@ -703,7 +703,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_canfd_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
@@ -719,7 +719,7 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_canxl_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..60406439a13f 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -54,6 +54,7 @@
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
@@ -1719,7 +1720,7 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 				goto out_release;
 			}
 
-			if (dev->type != ARPHRD_CAN) {
+			if (!can_get_ml_priv(dev)) {
 				dev_put(dev);
 				ret = -ENODEV;
 				goto out_release;
@@ -1867,7 +1868,7 @@ static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
@@ -2024,7 +2025,7 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 			ret = -ENODEV;
 			goto fail;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			dev_put(dev);
 			ret = -ENODEV;
 			goto fail;
diff --git a/net/can/gw.c b/net/can/gw.c
index 0ec99f68aa45..d9912dea738b 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -52,6 +52,7 @@
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/gw.h>
@@ -609,7 +610,7 @@ static int cgw_notifier(struct notifier_block *nb,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net *net = dev_net(dev);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 
 	if (msg == NETDEV_UNREGISTER) {
@@ -1160,7 +1161,7 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	if (!gwj->src.dev)
 		goto out;
 
-	if (gwj->src.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->src.dev))
 		goto out;
 
 	gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx);
@@ -1168,7 +1169,7 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	if (!gwj->dst.dev)
 		goto out;
 
-	if (gwj->dst.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->dst.dev))
 		goto out;
 
 	/* is sending the skb back to the incoming interface intended? */
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 155530aedce2..4560d5a567ec 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -65,6 +65,7 @@
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
@@ -1606,7 +1607,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		err = -ENODEV;
 		goto out;
 	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
 		err = -ENODEV;
 		goto out_put_dev;
 	}
@@ -1893,7 +1894,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
diff --git a/net/can/raw.c b/net/can/raw.c
index 82d9c0499c95..8e2e114abcb9 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -344,7 +344,7 @@ static int raw_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
@@ -487,7 +487,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 			err = -ENODEV;
 			goto out;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
-- 
2.53.0

[PATCH net 03/14] can: bittiming: fix divide-by-zero in can_calc_bittiming()

[Marc Kleine-Budde]

From: Sureshkumar S <ssureshmsd7@gmail.com>

can_calc_bittiming() scans the possible time segment combinations and
computes the prescaler for each of them as:

	brp = priv->clock.freq / (tsegall * bt->bitrate)

bt->bitrate is supplied by userspace via IFLA_CAN_BITTIMING and
tsegall * bt->bitrate is a 32 bit multiplication, so the product wraps
to zero as soon as bt->bitrate carries enough factors of two for the
tsegall values walked by the loop. The division then faults:

  Oops: divide error: 0000 [#1] SMP KASAN NOPTI
  RIP: 0010:can_calc_bittiming+0x32e/0xcc0
  Call Trace:
   can_changelink+0x8ba/0x2060
   __rtnl_newlink+0x1013/0x18a0
   rtnl_newlink+0x6b/0xa0
   rtnetlink_rcv_msg+0x6f9/0xb70
   netlink_rcv_skb+0x11f/0x350
   netlink_unicast+0x5f5/0x860
   netlink_sendmsg+0x70a/0xba0

This does not require an absurd bitrate. With the segment limits of a
typical controller tsegall reaches 256, so a bitrate of 16777216 is
already enough to wrap the product, and that value is below the
20 Mbit/s CAN XL data bitrate ceiling. Any CAN driver providing a
bittiming_const is affected; reproduced on dummy_can. Triggering it
needs CAP_NET_ADMIN in the netns owning the device.

priv->bitrate_max cannot guard against this: it is populated from the
optional "max-bitrate" device tree property, so it is zero for most
drivers, and can_changelink() only consults it after can_get_bittiming()
has already returned.

Compute the product with mul_u32_u32(), as can_fixup_bittiming() already
does for bt->brp * NSEC_PER_SEC, and divide with div64_u64(). div_u64()
cannot be used here because its divisor is a u32, which would truncate
the product back to the faulting value.

Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Sureshkumar S <ssureshmsd7@gmail.com>
Link: https://patch.msgid.link/20260803091426.29050-2-ssureshmsd7@gmail.com
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/calc_bittiming.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 42498e9d3f38..4809f5e0c96e 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -119,8 +119,12 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
 		tsegall = CAN_SYNC_SEG + tseg / 2;
 
-		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
-		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
+		/* Compute all possible tseg choices (tseg=tseg1+tseg2).
+		 * A 32 bit tsegall * bt->bitrate can wrap to zero for large
+		 * userspace bitrates, so compute the product in 64 bit.
+		 */
+		brp = div64_u64(priv->clock.freq,
+				mul_u32_u32(tsegall, bt->bitrate)) + tseg % 2;
 
 		/* choose brp step which is possible in system */
 		brp = (brp / btc->brp_inc) * btc->brp_inc;
-- 
2.53.0

[PATCH net 04/14] can: bittiming: fix bitrate error calculation on unsigned operands

[Marc Kleine-Budde]

From: Sureshkumar S <ssureshmsd7@gmail.com>

can_calc_bittiming() rates each candidate against the requested bitrate
with:

	bitrate_error = abs(bt->bitrate - bitrate);

Both operands are unsigned int, so the subtraction wraps instead of
becoming negative, and abs() resolves an unsigned int argument to its
int branch. A wrapped difference is therefore reinterpreted as a small
positive value instead of the large error it actually represents.

A requested bitrate far above anything the controller can reach then
passes the CAN_CALC_MAX_ERROR gate. On a dummy_can device with a 160 MHz
clock, requesting 4294967294 bps reports an error of 0.01%, configures
415584 bps and returns success to userspace, where -EINVAL is expected.

Use abs_diff(), which subtracts the smaller operand from the larger one
and keeps the whole comparison unsigned.

Fixes: 7da29f97d6c8 ("can: dev: can-calc-bit-timing(): better sample point calculation")
Signed-off-by: Sureshkumar S <ssureshmsd7@gmail.com>
Link: https://patch.msgid.link/20260803091426.29050-3-ssureshmsd7@gmail.com
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/calc_bittiming.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 4809f5e0c96e..2789b99ab6a8 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -132,7 +132,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 			continue;
 
 		bitrate = priv->clock.freq / (brp * tsegall);
-		bitrate_error = abs(bt->bitrate - bitrate);
+		bitrate_error = abs_diff(bt->bitrate, bitrate);
 
 		/* tseg brp biterror */
 		if (bitrate_error > best_bitrate_error)
-- 
2.53.0

[PATCH net 05/14] can: rockchip_canfd: prevent TX stall on echo skb failure

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_start_xmit() advances tx_head and requests transmission even when
can_put_echo_skb() fails. This creates a pending TX entry without the echo
skb that the RXSTX completion path needs to match the self-received frame.
The entry cannot be completed, and the netdev TX queue can remain stopped
after the two-entry software FIFO fills.

Install the echo skb before loading the hardware TX buffer. If installation
fails, account the frame as dropped and leave both the hardware FIFO and
software TX state unchanged. After the echo skb is installed, use the
stored echo skb as the source for the hardware frame data.

This depends on the standalone can_put_echo_skb() ownership fix. It makes
the remaining -EINVAL path consume the skb and was posted at:

Link: https://lore.kernel.org/linux-can/tencent_944DADCC4B42C8484EC01DA2B15F42132906@qq.com
Fixes: b6661d73290c ("can: rockchip_canfd: add TX PATH")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_C82C09E7235101CC88A97E154D2534183208@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 12200dcfd338..86fa8f2e1c8b 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -88,7 +88,16 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
-	cfd = (struct canfd_frame *)skb->data;
+	tx_head = rkcanfd_get_tx_head(priv);
+	frame_len = can_skb_get_frame_len(skb);
+	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
+	if (err) {
+		ndev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+
+	skb = priv->can.echo_skb[tx_head];
+	cfd = (const struct canfd_frame *)skb->data;
 
 	if (cfd->can_id & CAN_EFF_FLAG) {
 		reg_frameinfo = RKCANFD_REG_FD_FRAMEINFO_FRAME_FORMAT;
@@ -114,7 +123,6 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 					    cfd->len);
 	}
 
-	tx_head = rkcanfd_get_tx_head(priv);
 	reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
 
 	rkcanfd_write(priv, RKCANFD_REG_FD_TXFRAMEINFO, reg_frameinfo);
@@ -123,10 +131,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		rkcanfd_write(priv, RKCANFD_REG_FD_TXDATA0 + i,
 			      *(u32 *)(cfd->data + i));
 
-	frame_len = can_skb_get_frame_len(skb);
-	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
-	if (!err)
-		netdev_sent_queue(priv->ndev, frame_len);
+	netdev_sent_queue(priv->ndev, frame_len);
 
 	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
 
-- 
2.53.0

[PATCH net 06/14] can: rockchip_canfd: retry the outstanding TX buffer

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

rkcanfd_xmit_retry() originally operated with a TX FIFO depth of one. At
that depth, the masked head and tail indices both select buffer 0, so using
tx_head happened to select the correct buffer.

After the FIFO depth was increased to two, tx_head instead identifies the
next free buffer when one frame is outstanding. The erratum 6 workaround
therefore requests transmission from the wrong buffer, leaving the
outstanding echo entry incomplete and the netdev TX queue stopped.

Use tx_tail to select the outstanding buffer for retransmission.

Fixes: a5605d61c7dd ("can: rockchip_canfd: enable full TX-FIFO depth of 2")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_2CCE005FC440BF8026599A6999EC04BE1107@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/rockchip/rockchip_canfd-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index 86fa8f2e1c8b..fc338ea865fe 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -57,8 +57,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
 
 void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 {
-	const unsigned int tx_head = rkcanfd_get_tx_head(priv);
-	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
+	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
+	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
 
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
 }
-- 
2.53.0

[PATCH net 07/14] can: rockchip_canfd: serialize TX state and command writes

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

The TX completion path removes an echo skb before advancing tx_tail. In
parallel, the transmit path reads tx_head, tx_tail and the tail echo slot
when applying the erratum 6 queue restriction. There is no synchronization
between these operations.

On SMP, the transmit path can consequently observe a pending frame with
an empty echo slot. It can also keep a pointer to an echo skb while the
completion path removes and queues it for NAPI, where it can be freed on
another CPU. The inconsistent snapshot can stop the netdev TX queue when
there is no later completion to wake it.

There is a second race in the command submission sequence.
rkcanfd_start_xmit() runs in softirq context.
rkcanfd_xmit_retry() runs from the RX interrupt handler. On controllers
affected by erratum 12, both execute a MODE/CMD/MODE register sequence. The
interrupt handler can restore the default MODE between the softirq writes.
The resumed softirq then issues CMD without SPACE_RX_MODE and bypasses the
erratum 12 workaround.

Add a TX state lock and use it to protect tx_head, tx_tail and the echo skb
ring as one state. The same lock serializes the MODE/CMD/MODE sequence
between the transmit and interrupt paths. Keep completion and wakeup
outside the lock to avoid nesting the TX state lock with the netdev TX
queue lock. Install the echo skb before loading the hardware TX buffer so
an echo setup failure cannot desynchronize the hardware and software TX
state.

Tested on an RK3588 rev2.2 at 1 Mbit/s with 100,000 extended CAN frames.
The run triggered 138 erratum 6 retries and completed without drops, queue
stalls or driver warnings. RK3588 does not enable erratum 12, so this test
does not exercise that hardware workaround.

Fixes: ae002cc32ec4 ("can: rockchip_canfd: prepare to use full TX-FIFO depth")
Fixes: 83f9bd6bf39d ("can: rockchip_canfd: implement workaround for erratum 12")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_AF224EFBC9343DD238C80824AC8CA805480A@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/rockchip/rockchip_canfd-core.c    |  1 +
 drivers/net/can/rockchip/rockchip_canfd-rx.c  | 31 ++++++++++++++-----
 drivers/net/can/rockchip/rockchip_canfd-tx.c  | 26 ++++++++++++++--
 drivers/net/can/rockchip/rockchip_canfd.h     |  4 ++-
 4 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 37c1c22c40c9..1cae86973da5 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -921,6 +921,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
 	priv->can.do_set_mode = rkcanfd_set_mode;
 	priv->can.do_get_berr_counter = rkcanfd_get_berr_counter;
 	priv->ndev = ndev;
+	spin_lock_init(&priv->tx_lock);
 
 	match = device_get_match_data(&pdev->dev);
 	if (match) {
diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
index 24e87daa1df0..59420233c918 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
@@ -100,14 +100,25 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 	const struct canfd_frame *cfd_nominal;
 	const struct sk_buff *skb;
 	unsigned int tx_tail;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
+	if (!rkcanfd_get_tx_pending(priv))
+		goto out_unlock;
 
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 	if (!skb) {
+		const unsigned int tx_head_unmasked = priv->tx_head;
+		const unsigned int tx_tail_unmasked = priv->tx_tail;
+
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 		netdev_err(priv->ndev,
 			   "%s: echo_skb[%u]=NULL tx_head=0x%08x tx_tail=0x%08x\n",
 			   __func__, tx_tail,
-			   priv->tx_head, priv->tx_tail);
+			   tx_head_unmasked, tx_tail_unmasked);
 
 		return -ENOMSG;
 	}
@@ -123,17 +134,18 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 		rkcanfd_handle_tx_done_one(priv, ts, &frame_len);
 
 		WRITE_ONCE(priv->tx_tail, priv->tx_tail + 1);
+		*tx_done = true;
+
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
 		netif_subqueue_completed_wake(priv->ndev, 0, 1, frame_len,
 					      rkcanfd_get_effective_tx_free(priv),
 					      RKCANFD_TX_START_THRESHOLD);
 
-		*tx_done = true;
-
 		return 0;
 	}
 
 	if (!(priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6))
-		return 0;
+		goto out_unlock;
 
 	/* Erratum 6: Extended frames may be send as standard frames.
 	 *
@@ -143,7 +155,7 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 	 */
 	if (!(cfd_nominal->can_id & CAN_EFF_FLAG) ||
 	    (cfd_rx->can_id & CAN_EFF_FLAG))
-		return 0;
+		goto out_unlock;
 
 	/* Not affected if:
 	 * - standard part and RTR flag of the TX'ed frame
@@ -151,20 +163,20 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 	 */
 	if ((cfd_nominal->can_id & (CAN_RTR_FLAG | CAN_SFF_MASK)) !=
 	    (cfd_rx->can_id & (CAN_RTR_FLAG | CAN_SFF_MASK)))
-		return 0;
+		goto out_unlock;
 
 	/* Not affected if:
 	 * - length is not the same
 	 */
 	if (cfd_nominal->len != cfd_rx->len)
-		return 0;
+		goto out_unlock;
 
 	/* Not affected if:
 	 * - the data of non RTR frames is different
 	 */
 	if (!(cfd_nominal->can_id & CAN_RTR_FLAG) &&
 	    memcmp(cfd_nominal->data, cfd_rx->data, cfd_nominal->len))
-		return 0;
+		goto out_unlock;
 
 	/* Affected by Erratum 6 */
 	u64_stats_update_begin(&rkcanfd_stats->syncp);
@@ -185,6 +197,9 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
 
 	rkcanfd_xmit_retry(priv);
 
+out_unlock:
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 	return 0;
 }
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
index fc338ea865fe..b367341dd0ae 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
@@ -14,6 +14,8 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
 	const struct sk_buff *skb;
 	unsigned int tx_tail;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	if (!rkcanfd_get_tx_pending(priv))
 		return false;
 
@@ -33,13 +35,22 @@ static bool rkcanfd_tx_tail_is_eff(const struct rkcanfd_priv *priv)
 	return cfd->can_id & CAN_EFF_FLAG;
 }
 
-unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv)
+unsigned int rkcanfd_get_effective_tx_free(struct rkcanfd_priv *priv)
 {
+	unsigned int tx_free;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
 	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_6 &&
 	    rkcanfd_tx_tail_is_eff(priv))
-		return 0;
+		tx_free = 0;
+	else
+		tx_free = rkcanfd_get_tx_free(priv);
 
-	return rkcanfd_get_tx_free(priv);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	return tx_free;
 }
 
 static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
@@ -60,6 +71,8 @@ void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
 	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
 	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
 }
 
@@ -69,6 +82,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 reg_frameinfo, reg_id, reg_cmd;
 	unsigned int tx_head, frame_len;
 	const struct canfd_frame *cfd;
+	unsigned long flags;
 	int err;
 	u8 i;
 
@@ -88,10 +102,13 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
+	spin_lock_irqsave(&priv->tx_lock, flags);
 	tx_head = rkcanfd_get_tx_head(priv);
 	frame_len = can_skb_get_frame_len(skb);
 	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
 	if (err) {
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+
 		ndev->stats.tx_dropped++;
 		return NETDEV_TX_OK;
 	}
@@ -136,6 +153,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
 
 	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
 
 	netif_subqueue_maybe_stop(priv->ndev, 0,
 				  rkcanfd_get_effective_tx_free(priv),
@@ -152,6 +170,8 @@ void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
 	unsigned int tx_tail;
 	struct sk_buff *skb;
 
+	lockdep_assert_held(&priv->tx_lock);
+
 	tx_tail = rkcanfd_get_tx_tail(priv);
 	skb = priv->can.echo_skb[tx_tail];
 
diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
index 95bea9bfd8a2..2dc3a41afe32 100644
--- a/drivers/net/can/rockchip/rockchip_canfd.h
+++ b/drivers/net/can/rockchip/rockchip_canfd.h
@@ -15,6 +15,7 @@
 #include <linux/netdevice.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
+#include <linux/spinlock.h>
 #include <linux/timecounter.h>
 #include <linux/types.h>
 #include <linux/u64_stats_sync.h>
@@ -474,6 +475,7 @@ struct rkcanfd_priv {
 	struct can_rx_offload offload;
 	struct net_device *ndev;
 
+	spinlock_t tx_lock; /* protects tx_head, tx_tail and echo_skb */
 	void __iomem *regs;
 	unsigned int tx_head;
 	unsigned int tx_tail;
@@ -556,7 +558,7 @@ void rkcanfd_timestamp_start(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop(struct rkcanfd_priv *priv);
 void rkcanfd_timestamp_stop_sync(struct rkcanfd_priv *priv);
 
-unsigned int rkcanfd_get_effective_tx_free(const struct rkcanfd_priv *priv);
+unsigned int rkcanfd_get_effective_tx_free(struct rkcanfd_priv *priv);
 void rkcanfd_xmit_retry(struct rkcanfd_priv *priv);
 netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev);
 void rkcanfd_handle_tx_done_one(struct rkcanfd_priv *priv, const u32 ts,
-- 
2.53.0

[PATCH net 08/14] can: skb: make echo skb freeing safe in any IRQ context

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

can_put_echo_skb() can be called with hardware interrupts disabled. Its
direct drop paths use kfree_skb(), while can_create_echo_skb() uses
kfree_skb() when cloning fails and consume_skb() after a successful clone.
None of these helpers is safe in every IRQ context.

Use dev_kfree_skb_any() for all drop paths and dev_consume_skb_any() when
consuming a successfully cloned skb. This preserves the respective skb drop
and consumed semantics regardless of the caller IRQ context.

Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 4 ++--
 include/linux/can/skb.h   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 4f7a189de265..0ed8057a3cb2 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	    (skb->protocol != htons(ETH_P_CAN) &&
 	     skb->protocol != htons(ETH_P_CANFD) &&
 	     skb->protocol != htons(ETH_P_CANXL))) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return 0;
 	}
 
@@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	} else {
 		/* locking problem with netif_stop_queue() ?? */
 		netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx);
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return -EBUSY;
 	}
 
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index a70a02967071..78c5870e2f9a 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -76,12 +76,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
 
 	nskb = skb_clone(skb, GFP_ATOMIC);
 	if (unlikely(!nskb)) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return NULL;
 	}
 
 	can_skb_set_owner(nskb, skb->sk);
-	consume_skb(skb);
+	dev_consume_skb_any(skb);
 	return nskb;
 }
 
-- 
2.53.0

[PATCH net 09/14] can: skb: make CAN skb allocation failure paths IRQ-safe

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

The CAN skb allocation helpers are used from hardware interrupt receive
handlers. If can_skb_ext_add() fails, they release the newly allocated skb
with kfree_skb(), which is not safe in hardware interrupt context.

Use dev_kfree_skb_any() for the allocation failure paths in
alloc_can_skb(), alloc_canfd_skb(), and alloc_canxl_skb().

Fixes: 96ea3a1e2d31 ("can: add CAN skb extension infrastructure")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_C825C17D442F801351CE2FBC4984064B4605@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 0ed8057a3cb2..c7be21ed70dd 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -224,7 +224,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_cc;
 	}
 
@@ -255,7 +255,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_fd;
 	}
 
@@ -293,7 +293,7 @@ struct sk_buff *alloc_canxl_skb(struct net_device *dev,
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_xl;
 	}
 
-- 
2.53.0

[PATCH net 10/14] can: dev: can_put_echo_skb(): free skb on invalid echo index

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

can_put_echo_skb() consumes the skb on all paths except when the echo
index is out of bounds. This leaves ownership with the caller on -EINVAL,
unlike the other error paths, and can leak the skb if the caller expects
consistent semantics.

Free the skb before returning -EINVAL so that all return paths consume it.

Fixes: 6411959c10fe ("can: dev: can_put_echo_skb(): don't crash kernel if can_priv::echo_skb is accessed out of bounds")
Cc: stable@vger.kernel.org
Reviewed-by: Vincent Mailhol <mailhol@kernel.org>
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index c7be21ed70dd..faafb1033d42 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -55,6 +55,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
 			   __func__, idx, priv->echo_skb_max);
+		dev_kfree_skb_any(skb);
 		return -EINVAL;
 	}
 
-- 
2.53.0

[PATCH net 11/14] can: kvaser_pciefd: fix use-after-free in bec poll timer

[Marc Kleine-Budde]

From: Fan Wu <fanwu01@zju.edu.cn>

The bec poll timer is rearmed from the interrupt handler, so the
timer_delete() call in kvaser_pciefd_remove() neither waits for a
callback that is already running nor stops the handler from rearming
the timer until the interrupt is freed later in the same function.
The timer can therefore still be pending or running when free_candev()
frees the CAN device, causing a use-after-free in
kvaser_pciefd_bec_poll_timer().

Use timer_shutdown_sync() instead, which waits for a running callback
and makes a later rearm a no-op.  Also drain the timer in
kvaser_pciefd_teardown_can_ctrls(), which frees the CAN devices on the
probe error paths.

This issue was found by an in-house static analysis tool.

Fixes: 26ad340e582d ("can: kvaser_pciefd: Add driver for Kvaser PCIEcan devices")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
Link: https://patch.msgid.link/20260818063832.383829-1-fanwu01@zju.edu.cn
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
index d8c9bfb20230..a0597db72086 100644
--- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
+++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
@@ -1739,6 +1739,7 @@ static void kvaser_pciefd_teardown_can_ctrls(struct kvaser_pciefd *pcie)
 			iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
 			kvaser_pciefd_pwm_stop(can);
 			kvaser_pciefd_devlink_port_unregister(can);
+			timer_shutdown_sync(&can->bec_poll_timer);
 			free_candev(can->can.dev);
 		}
 	}
@@ -1879,7 +1880,7 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)
 		struct kvaser_pciefd_can *can = pcie->can[i];
 
 		unregister_candev(can->can.dev);
-		timer_delete(&can->bec_poll_timer);
+		timer_shutdown_sync(&can->bec_poll_timer);
 		kvaser_pciefd_pwm_stop(can);
 		kvaser_pciefd_devlink_port_unregister(can);
 	}
-- 
2.53.0

[PATCH net 12/14] can: kvaser_usb: validate command format before parsing in hydra receive path

[Marc Kleine-Budde]

From: "Cen Zhang (Microsoft)" <blbllhy@gmail.com>

The receive-path command parsers (kvaser_usb_hydra_wait_cmd and
kvaser_usb_hydra_read_bulk_callback) call kvaser_usb_hydra_cmd_size()
without verifying that enough buffer remains.  For CMD_EXTENDED,
kvaser_usb_hydra_cmd_size() unconditionally reads a 2-byte len field
at offset 4 (kvaser_usb_hydra.c:532).  A malicious USB device can place
a CMD_EXTENDED header at the end of a 3072-byte bulk transfer such that
only 4 bytes remain, causing a 2-byte slab-out-of-bounds read.

  BUG: KASAN: slab-out-of-bounds in kvaser_usb_hydra_wait_cmd+0x3f1/0x480
    [kvaser_usb_hydra.c:678]
  Read of size 2 at addr ffff888013f7ec00 by task kworker/0:0/9
   kvaser_usb_hydra_wait_cmd+0x3f1/0x480
   kvaser_usb_hydra_get_software_details+0x1c7/0x5d0
   kvaser_usb_probe+0x36a/0x1240

Additionally, if the device sends CMD_EXTENDED with len=0,
kvaser_usb_hydra_cmd_size() returns 0 and the parser loops forever
(pos += 0), permanently burning one CPU core.

Fix by adding kvaser_usb_hydra_cmd_size_rx(), which checks whether the
size field is complete before reading it and rejects zero command lengths.
The asynchronous receive path preserves incomplete headers in the leftover
buffer and completes them from the following transfer.  Clear malformed
leftover state before returning so later transfers do not retry it.

Fixes: aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")
Reported-by: AutonomousCodeSecurity@microsoft.com
Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
Link: https://patch.msgid.link/20260826013037.6933-1-blbllhy@gmail.com
[mkl: reduce scope of err in kvaser_usb_hydra_read_bulk_callback()]
[mkl: increase readability, reformat to make use of ~100 columns]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 62 ++++++++++++++++---
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index efbb7bed34c9..efe7b3ef489b 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -536,6 +536,24 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
 	return ret;
 }
 
+/* -EAGAIN means incomplete; -EINVAL rejects zero to ensure progress. */
+static int kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
+					size_t remaining, size_t *cmd_len)
+{
+	if (remaining < sizeof(cmd->header.cmd_no))
+		return -EAGAIN;
+
+	if (cmd->header.cmd_no == CMD_EXTENDED &&
+	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
+		return -EAGAIN;
+
+	*cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+	if (!*cmd_len)
+		return -EINVAL;
+
+	return 0;
+}
+
 static struct kvaser_usb_net_priv *
 kvaser_usb_hydra_net_priv_from_cmd(const struct kvaser_usb *dev,
 				   const struct kvaser_cmd *cmd)
@@ -675,8 +693,8 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
 			size_t cmd_len;
 
 			tmp_cmd = buf + pos;
-			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
-			if (pos + cmd_len > actual_len) {
+			err = kvaser_usb_hydra_cmd_size_rx(tmp_cmd, actual_len - pos, &cmd_len);
+			if (err || pos + cmd_len > actual_len) {
 				dev_err_ratelimited(&dev->intf->dev,
 						    "Format error\n");
 				break;
@@ -2120,23 +2138,47 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 	spin_lock_irqsave(usb_rx_leftover_lock, irq_flags);
 	usb_rx_leftover_len = card_data->usb_rx_leftover_len;
 	if (usb_rx_leftover_len) {
+		const size_t cmd_size_field_end = offsetof(struct kvaser_cmd_ext, cmd_no_ext);
 		int remaining_bytes;
+		int err;
 
 		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
 
-		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		if (cmd->header.cmd_no == CMD_EXTENDED &&
+		    usb_rx_leftover_len < cmd_size_field_end) {
+			remaining_bytes = min_t(int, len, cmd_size_field_end - usb_rx_leftover_len);
 
-		remaining_bytes = min_t(unsigned int, len,
+			memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf, remaining_bytes);
+			usb_rx_leftover_len += remaining_bytes;
+			card_data->usb_rx_leftover_len = usb_rx_leftover_len;
+			pos += remaining_bytes;
+
+			if (usb_rx_leftover_len < cmd_size_field_end) {
+				spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
+				return;
+			}
+		}
+
+		err = kvaser_usb_hydra_cmd_size_rx(cmd, usb_rx_leftover_len, &cmd_len);
+		if (err || cmd_len < usb_rx_leftover_len) {
+			dev_err(&dev->intf->dev, "Format error\n");
+			card_data->usb_rx_leftover_len = 0;
+			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
+			return;
+		}
+
+		remaining_bytes = min_t(unsigned int, len - pos,
 					cmd_len - usb_rx_leftover_len);
 		/* Make sure we do not overflow usb_rx_leftover */
 		if (remaining_bytes + usb_rx_leftover_len >
 						KVASER_USB_HYDRA_MAX_CMD_LEN) {
 			dev_err(&dev->intf->dev, "Format error\n");
+			card_data->usb_rx_leftover_len = 0;
 			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
 			return;
 		}
 
-		memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf,
+		memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf + pos,
 		       remaining_bytes);
 		pos += remaining_bytes;
 
@@ -2152,11 +2194,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 	spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
 
 	while (pos < len) {
+		int err;
+
 		cmd = buf + pos;
 
-		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		err = kvaser_usb_hydra_cmd_size_rx(cmd, len - pos, &cmd_len);
+		if (err && err != -EAGAIN) {
+			dev_err(&dev->intf->dev, "Format error\n");
+			return;
+		}
 
-		if (pos + cmd_len > len) {
+		if (err == -EAGAIN || pos + cmd_len > len) {
 			/* We got first part of a command */
 			int leftover_bytes;
 
-- 
2.53.0

[PATCH net 13/14] can: usb: f81604: fix struct f81604_int_data size mismatch

[Marc Kleine-Budde]

From: "Ji-Ze Hong (Peter Hong)" <peter_hong@fintek.com.tw>

The struct f81604_int_data defines 9 bytes of interrupt data:
- Byte 0: Status register (sr)
- Byte 1: Interrupt register (isrc)
- Byte 2: Interrupt enable register (ier)
- Byte 3: Arbitration lost capture (alc)
- Byte 4: Error code capture (ecc)
- Byte 5: Error warning limit register (ewlr)
- Byte 6: RX error counter (rxerr)
- Byte 7: TX error counter (txerr)
- Byte 8: Reserved (val)

The hardware sends exactly 9 bytes for the interrupt endpoint.
However, the struct was defined with __aligned(4) attribute which
caused the compiler to pad the struct to 12 bytes.

This causes a problem in f81604_read_int_callback() where the short
URB check compares urb->actual_length against sizeof(*data). When
sizeof(struct f81604_int_data) is 12 but the hardware only sends 9
bytes, the check fails and valid interrupt messages are discarded.

This results in the driver only being able to transmit once because
the TX complete interrupt is never processed.

Fix this by removing the __aligned(4) attribute so the struct size
matches the actual hardware data size of 9 bytes.

Fixes: 7299b1b39a25 ("can: usb: f81604: handle short interrupt urb messages properly")
Cc: stable@vger.kernel.org
Reported-by: Dynetrex, Admin <admin@dynetrex.com>
Closes: https://lore.kernel.org/all/A3834A07-5639-4779-844F-C5843DFC3928@dynetrex.com/
Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Drew Willey <dwilley@google.com>
Link: https://patch.msgid.link/20260824-f81604-fix-v2-1-fc9be5581394@fintek.com.tw
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/f81604.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
index f12318268e46..4c147b9d6d69 100644
--- a/drivers/net/can/usb/f81604.c
+++ b/drivers/net/can/usb/f81604.c
@@ -169,7 +169,7 @@ struct f81604_int_data {
 	u8 rxerr;
 	u8 txerr;
 	u8 val;
-} __packed __aligned(4);
+} __packed;
 
 struct f81604_sff {
 	__be16 id;
-- 
2.53.0

[PATCH net 14/14] can: hi311x: drop hi3110_lock before free_irq() on open failure

[Marc Kleine-Budde]

From: Runyu Xiao <runyu.xiao@seu.edu.cn>

hi3110_open() requests a threaded IRQ and then performs hardware
setup while holding priv->hi3110_lock. If reset, setup, or
normal-mode entry fails, the error path calls free_irq() while still
holding that mutex.

The threaded handler takes priv->hi3110_lock before checking
force_quit, while free_irq() waits for the threaded handler to finish.
That can deadlock the open() rollback path against a pending IRQ
thread.

Set force_quit, drop hi3110_lock before free_irq(), and take the
mutex again for the remaining hardware cleanup.

Fixes: 57e83fb9b746 ("can: hi311x: Add Holt HI-311x CAN driver")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
Link: https://patch.msgid.link/20260820020631.316418-1-runyu.xiao@seu.edu.cn
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/hi311x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index ae90e6716de5..2be851e8907d 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -787,7 +787,10 @@ static int hi3110_open(struct net_device *net)
 	return 0;
 
  out_free_irq:
+	priv->force_quit = 1;
+	mutex_unlock(&priv->hi3110_lock);
 	free_irq(spi->irq, priv);
+	mutex_lock(&priv->hi3110_lock);
 	hi3110_hw_sleep(spi);
  out_close:
 	hi3110_power_enable(priv->transceiver, 0);
-- 
2.53.0

Re: [PATCH net 08/14] can: skb: make echo skb freeing safe in any IRQ context

[sashiko-bot]

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

Pre-existing issues:
- [High] Unsafe kfree_skb() call in SKB validation drop paths from hardirq disabled context.
--

commit e51772559e06a8eadc0f2ac2b7cdadfcaeb3e18b
Author: Cunhao Lu <1579567540@qq.com>

can: skb: make echo skb freeing safe in any IRQ context

can_put_echo_skb() can be called with hardware interrupts disabled. Its
direct drop paths use kfree_skb(), while can_create_echo_skb() uses
kfree_skb() when cloning fails and consume_skb() after a successful clone.
None of these helpers is safe in every IRQ context.

Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 4f7a189de265d..0ed8057a3cb2c 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	    (skb->protocol != htons(ETH_P_CAN) &&
>  	     skb->protocol != htons(ETH_P_CANFD) &&
>  	     skb->protocol != htons(ETH_P_CANXL))) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return 0;
>  	}
>  
> @@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	} else {
>  		/* locking problem with netif_stop_queue() ?? */
>  		netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx);
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return -EBUSY;
>  	}
>  
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index a70a029670711..78c5870e2f9a2 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -76,12 +76,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
>  
>  	nskb = skb_clone(skb, GFP_ATOMIC);
>  	if (unlikely(!nskb)) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return NULL;
>  	}
>  
>  	can_skb_set_owner(nskb, skb->sk);
> -	consume_skb(skb);
> +	dev_consume_skb_any(skb);
>  	return nskb;
>  }

[Severity: High]
This is a pre-existing issue, but since this patch fixes context violations
in can_put_echo_skb() (which is called from ndo_start_xmit with hardirqs
disabled, e.g. via netpoll), does it make sense to fix the identical
violations in the adjacent CAN drop paths?

When a CAN driver's ndo_start_xmit rejects an invalid packet, it calls
can_dev_dropped_skb(), which still uses the IRQ-unsafe kfree_skb():

include/linux/can/dev.h:can_dev_dropped_skb() {
    ...
invalid_skb:
	kfree_skb(skb);
	dev->stats.tx_dropped++;
	return true;
}

Similarly, can_dropped_invalid_skb() drops invalid SKBs using kfree_skb(),
which can trigger a kernel oops or context warning if called when hardirqs
are disabled:

drivers/net/can/dev/skb.c:can_dropped_invalid_skb() {
    ...
inval_skb:
	kfree_skb(skb);
	dev->stats.tx_dropped++;
	return true;
}

Can these pre-existing drop paths also be updated to use dev_kfree_skb_any()
to prevent crashes when malformed packets are transmitted while netpoll is
active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=8

Re: [PATCH net 10/14] can: dev: can_put_echo_skb(): free skb on invalid echo index

[sashiko-bot]

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

Pre-existing issues:
- [High] Modifying SKB payload in can_skb_init_valid() without unsharing causes data corruption for concurrent observers.
--

commit 6a48fe2d96b6f1ca706da34b2f7bdd872d06941f
Author: Cunhao Lu <1579567540@qq.com>
can: dev: can_put_echo_skb(): free skb on invalid echo index

This patch fixes a potential leak in can_put_echo_skb() when the echo index
is out of bounds. It ensures the socket buffer is freed before returning
-EINVAL so that all error paths correctly consume the SKB.

Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index c7be21ed70ddf..faafb1033d423 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but already existed in the
codebase. Does can_skb_init_valid() in this same file modify shared memory
without unsharing it first?

drivers/net/can/dev/skb.c:can_skb_init_valid() {
        ...
        /* set CANFD_FDF flag for CAN FD frames */
        if (can_is_canfd_skb(skb)) {
            struct canfd_frame *cfd;

            cfd = (struct canfd_frame *)skb->data;
            cfd->flags |= CANFD_FDF;
        }
        ...
}

If a user with CAP_NET_RAW injects a CAN FD frame via an AF_PACKET socket,
the packet goes to dev_queue_xmit() where it may be cloned for packet
sniffers like tcpdump.

When the original SKB reaches the driver's transmit function and calls
can_dev_dropped_skb() -> can_dropped_invalid_skb() -> can_skb_init_valid(),
it modifies the payload (cfd->flags |= CANFD_FDF) without verifying
skb_cloned(skb) or calling skb_unshare().

Can this cause data corruption for concurrent observers receiving the clones?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=10

Re: [PATCH net 11/14] can: kvaser_pciefd: fix use-after-free in bec poll timer

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing dma_rmb() when reading packets from coherent DMA memory.
- [High] Incomplete timer teardown and missing state checks in ndo_stop (kvaser_pciefd_stop) causes a polling loop leak, state corruption, and potential spurious netif_rx calls when the interface is brought down.
--

commit d6148cc43f3a9adbcbc3d9812cbf816bc0ffca1f
Author: Fan Wu <fanwu01@zju.edu.cn>

can: kvaser_pciefd: fix use-after-free in bec poll timer

This commit addresses a use-after-free by switching to timer_shutdown_sync()
in kvaser_pciefd_remove() and kvaser_pciefd_teardown_can_ctrls(). It ensures
the bec_poll_timer callback completes before freeing the CAN device during
module removal or probe errors.

Link: https://patch.msgid.link/20260818063832.383829-1-fanwu01@zju.edu.cn

> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> index d8c9bfb202304..a0597db720864 100644
> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c

[ ... ]

> @@ -1879,7 +1880,7 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)
>  		struct kvaser_pciefd_can *can = pcie->can[i];
>  
>  		unregister_candev(can->can.dev);
> -		timer_delete(&can->bec_poll_timer);
> +		timer_shutdown_sync(&can->bec_poll_timer);

[Severity: High]
This is a pre-existing issue, but since you are fixing the timer teardown here,
does the timer also need to be synced during ndo_stop?

In kvaser_pciefd_stop(), the code uses the non-sync timer_delete:

	iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
	timer_delete(&can->bec_poll_timer);
	can->can.state = CAN_STATE_STOPPED;

Because this does not wait for the callback to finish, couldn't a pending status
request be issued to the hardware right as the interface is brought down?

Furthermore, if a status response arrives via the shared PCIe interrupt after
the interface is stopped, kvaser_pciefd_handle_status_packet() and
kvaser_pciefd_handle_status_resp() do not seem to check if the device was
intentionally stopped. They overwrite the state to an active state and
unconditionally re-arm the timer:

drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_handle_status_packet() {
    ...
	if (can->can.state != CAN_STATE_BUS_OFF &&
	    can->can.state != CAN_STATE_ERROR_ACTIVE) {
		mod_timer(&can->bec_poll_timer, KVASER_PCIEFD_BEC_POLL_FREQ);
	}
    ...
}

Could this lead to an infinite polling loop leak and potential spurious
netif_rx calls on a stopped interface?

[Severity: Critical]
This is also a pre-existing issue, but I noticed a potential memory ordering
issue in kvaser_pciefd_read_packet().

When reading from the coherent DMA buffer:

drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_read_packet() {
    ...
	size = le32_to_cpu(buffer[pos++]);
	if (!size) {
		*start_pos = 0;
		return 0;
	}

	p->header[0] = le32_to_cpu(buffer[pos++]);
    ...
}

Is a dma_rmb() needed before reading the packet contents? 

Without a read memory barrier, weakly ordered CPU architectures might
speculatively read the header and payload before the size read completes.
If the hardware is actively updating the buffer, couldn't the CPU see the
new size but process stale header or payload data that was speculatively
prefetched?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=11

Re: [PATCH net 03/14] can: bittiming: fix divide-by-zero in can_calc_bittiming()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds checking on `bittiming_const` parameters allows malicious USB devices to trigger divide-by-zero crashes or infinite loops in `can_calc_bittiming()`.
--

commit 0fc6576c2e9b35990a8bf86f43e72c0d477992ea
Author: Sureshkumar S <ssureshmsd7@gmail.com>

can: bittiming: fix divide-by-zero in can_calc_bittiming()

This commit prevents a divide-by-zero panic in can_calc_bittiming() when
evaluating prescalers for large bitrates. It does this by computing the
product of tsegall and bt->bitrate in 64 bits to avoid wrap-around.

Link: https://patch.msgid.link/20260803091426.29050-2-ssureshmsd7@gmail.com

> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 42498e9d3f38d..4809f5e0c96e9 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c
> @@ -119,8 +119,12 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {

[Severity: High]
This is a pre-existing issue, but if a malicious CAN USB device (such as one
using the gs_usb driver) provides a USB descriptor where btc->tseg1_min and
btc->tseg2_min are both 0, the unsigned loop condition (tseg >= 0) becomes
always true.

Could this result in an integer wrap-around and billions of loop iterations,
leading to a system freeze (soft lockup)?

Since can_bittiming_const_valid() in drivers/net/can/dev/dev.c validates these
constants, should it be updated to verify that tseg1_min and tseg2_min are
non-zero?

>  		tsegall = CAN_SYNC_SEG + tseg / 2;
>  
> -		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
> -		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
> +		/* Compute all possible tseg choices (tseg=tseg1+tseg2).
> +		 * A 32 bit tsegall * bt->bitrate can wrap to zero for large
> +		 * userspace bitrates, so compute the product in 64 bit.
> +		 */
> +		brp = div64_u64(priv->clock.freq,
> +				mul_u32_u32(tsegall, bt->bitrate)) + tseg % 2;
>  
>  		/* choose brp step which is possible in system */
>  		brp = (brp / btc->brp_inc) * btc->brp_inc;

[Severity: High]
This isn't a bug introduced by this patch, but if btc->brp_inc is 0 due to an
unvalidated value from the USB device, would this calculation cause an
immediate divide-by-zero panic?

Additionally, if btc->brp_min is 0, brp can evaluate to 0 in some loop
iterations. Looking further down in can_calc_bittiming():

		bitrate = priv->clock.freq / (brp * tsegall);

Can this lead to another divide-by-zero in the same loop?

Similarly, if fclk_can (mapped to priv->clock.freq) is provided as 0 by the
device, would the following call near the end of can_calc_bittiming() also
result in a divide-by-zero panic?

	do_div(v64, priv->clock.freq);

Would it make sense to enhance can_bittiming_const_valid() to ensure that
parameters like brp_inc, brp_min, and clock.freq are strictly greater than
zero before they are used here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=3

Re: [PATCH net 02/14] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

[sashiko-bot]

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

Pre-existing issues:
- [High] Modifying cloned SKB data without unsharing causes data races and potential silent data corruption in shared CAN FD buffers.
--

commit cd014a29d1194dd4eff54897a5d2062f2ec32a0c
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
    
This patch converts unreliable ARPHRD_CAN type checks to use the robust
can_get_ml_priv() function. This prevents TUN/TAP devices from spoofing
their hardware type to bypass CAN device validation.

Link: https://patch.msgid.link/20260819160822.8256-1-socketcan@hartkopp.net

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 7bc86b176b4de..ef435f22ac933 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -226,7 +226,7 @@ int can_send(struct sk_buff *skb, int loop)
>  		goto inval_skb;
>  	}
>  
> -	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
> +	if (unlikely(!can_get_ml_priv(skb->dev))) {

[Severity: High]
This is a pre-existing issue, but while reviewing the changes in can_send(),
I noticed a potential data race when handling cloned SKBs.

When the CAN gateway forwards a frame without modification functions,
can_can_gw_rcv() forwards a cloned SKB:

net/can/gw.c:can_can_gw_rcv() {
    ...
    nskb = skb_clone(skb, GFP_ATOMIC);
    ...
    can_send(nskb, gwj->limit_hops);
}

Then in can_send() just above this diff hunk, the SKB data area is modified
unconditionally for CAN FD frames:

net/can/af_can.c:can_send() {
    ...
    } else if (can_is_canfd_skb(skb)) {
        struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
        ...
        /* set CAN FD flag for CAN FD frames by default */
        cfd->flags |= CANFD_FDF;
    }
    ...
}

Since skb_clone() shares the data area with the original SKB, does modifying
cfd->flags without unsharing the SKB first cause a data race?

On architectures without atomic byte-store instructions, this performs a
word-sized read-modify-write operation. If the original SKB is concurrently
processed by other receivers, could this silently corrupt adjacent fields?
Given that an unprivileged user with CAP_NET_ADMIN in a user namespace can
set up a virtual CAN interface and trigger this path, is this safe?

>  		err = -EPERM;
>  		goto inval_skb;
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=2

Re: [PATCH net 12/14] can: kvaser_usb: validate command format before parsing in hydra receive path

[sashiko-bot]

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

New issues:
- [High] The patch prevents a specific out-of-bounds read of `cmd->len` by verifying at least 6 bytes are available, but it fails to enforce a safe minimum structural length for extended commands. A malicious USB device can provide a trivially small `cmd->len` (e.g., 8), bypassing the buffer boundary checks in the parser and leading to severe slab-out-of-bounds reads in downstream command handlers.

Pre-existing issues:
- [Critical] Missing validation of the `echo_index` sentinel value allows a malicious USB device to trigger a slab-out-of-bounds read and an arbitrary kernel pointer write, leading to potential privilege escalation.
--

commit 36da6d5bcb0fd8b842d165066e47c1ff8b40c308
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

can: kvaser_usb: validate command format before parsing in hydra receive path

This commit adds bounds checking for command formats in the kvaser_usb hydra
receive path. It aims to prevent a slab-out-of-bounds read caused by malicious
USB devices sending truncated headers or zero-length commands.

Link: https://patch.msgid.link/20260826013037.6933-1-blbllhy@gmail.com

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> index efbb7bed34c9d..efe7b3ef489bf 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -536,6 +536,24 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
>  	return ret;
>  }
>  
> +/* -EAGAIN means incomplete; -EINVAL rejects zero to ensure progress. */
> +static int kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
> +					size_t remaining, size_t *cmd_len)
> +{
> +	if (remaining < sizeof(cmd->header.cmd_no))
> +		return -EAGAIN;
> +
> +	if (cmd->header.cmd_no == CMD_EXTENDED &&
> +	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
> +		return -EAGAIN;
> +
> +	*cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +	if (!*cmd_len)
> +		return -EINVAL;

[Severity: High]
Does this length validation allow maliciously small extended commands?

While this prevents cmd_len from being 0, it allows trivially small values
(e.g., 8 bytes). If a malicious device places an 8-byte extended command at
the very end of a 3072-byte USB bulk transfer, kvaser_usb_hydra_cmd_size_rx()
will return 0 and set cmd_len to 8. Because pos + 8 == len, the overarching
boundary check in the parser will pass, handing the 8-byte command to
kvaser_usb_hydra_handle_cmd().

Downstream handlers like kvaser_usb_hydra_rx_msg_ext() then blindly access
payload fields extending far beyond 8 bytes (such as cmd->rx_can.kcan_header
at offset 20). Does this code read out-of-bounds of the 3072-byte heap
allocation?

> +
> +	return 0;
> +}
> +
>  static struct kvaser_usb_net_priv *
>  kvaser_usb_hydra_net_priv_from_cmd(const struct kvaser_usb *dev,
>  				   const struct kvaser_cmd *cmd)

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a pre-existing issue where an unvalidated echo_index can lead to
an out-of-bounds read and arbitrary memory corruption.

In kvaser_usb_hydra_tx_acknowledge():

	u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
	...
	context = &priv->tx_contexts[transid % dev->max_tx_urbs];
	...
	skb = priv->can.echo_skb[context->echo_index];
	if (skb)
		skb_hwtstamps(skb)->hwtstamp = kvaser_usb_hydra_ktime_from_cmd(dev->cfg, cmd);

When a TX context is free (or newly initialized), context->echo_index is
set to the sentinel value dev->max_tx_urbs:

	context->echo_index = dev->max_tx_urbs;

If a malicious USB device sends a spoofed CMD_TX_ACKNOWLEDGE with a transid
matching a free TX context, this code directly indexes priv->can.echo_skb
using dev->max_tx_urbs.

Since echo_skb is placed exactly at the end of the net_device memory block
(via alloc_candev_mqs), an index of max_tx_urbs reads 8 bytes of garbage
from the subsequent kernel slab object.

If the read value is non-NULL, the driver dereferences it and writes a
64-bit timestamp derived from the attacker-controlled USB packet into memory.
Can this be triggered by a malicious USB device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=12

Re: [PATCH net 13/14] can: usb: f81604: fix struct f81604_int_data size mismatch

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect ordering of URB length and status checks causes fatal URB errors to be bypassed, leading to infinite resubmission loops or spurious warnings on device disconnect.
--

commit 7de2dcdbebd610316c61044116af8d2dda19bd1f
Author: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>

can: usb: f81604: fix struct f81604_int_data size mismatch

This commit removes the __aligned(4) attribute from struct f81604_int_data,
changing its size from 12 bytes back to 9 bytes. This ensures that the valid
9-byte interrupt URBs sent by the hardware are no longer incorrectly dropped
by the short URB check.

Link: https://patch.msgid.link/20260824-f81604-fix-v2-1-fc9be5581394@fintek.com.tw
Closes: https://lore.kernel.org/all/A3834A07-5639-4779-844F-C5843DFC3928@dynetrex.com/

> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> index f12318268e46b..4c147b9d6d690 100644
> --- a/drivers/net/can/usb/f81604.c
> +++ b/drivers/net/can/usb/f81604.c
> @@ -169,7 +169,7 @@ struct f81604_int_data {
>  	u8 rxerr;
>  	u8 txerr;
>  	u8 val;
> -} __packed __aligned(4);
> +} __packed;

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the ordering of
the URB length and status checks in f81604_read_int_callback().

When a hardware error occurs or the device is disconnected, the USB core
completes the interrupt URB with a fatal status (like -EPROTO or -ESHUTDOWN)
and sets urb->actual_length to 0.

Because f81604_read_int_callback() checks the length before the status:

	if (urb->actual_length < sizeof(*data)) {
		netdev_warn(netdev, "%s: short int URB: %u < %zu\n",
			    __func__, urb->actual_length, sizeof(*data));
		goto resubmit_urb;
	}

	switch (urb->status) {
	...
	case -ESHUTDOWN:
		return;
	...

Will this cause fatal URB errors to bypass the intended abort logic and loop
infinitely into resubmit_urb, or produce spurious warnings upon device
disconnect?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=13

Re: [PATCH net 14/14] can: hi311x: drop hi3110_lock before free_irq() on open failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of hi3110_clean() without mutual exclusion can result in a double-free of priv->tx_skb and a data race on priv->tx_busy.
- [High] hi3110_stop() does not cancel pending driver workqueues (priv->restart_work and priv->tx_work), leading to use-after-free, IRQ storms, or hardware access after power off.
--

commit 22aa36b3ba9420685fbf25a2a6f3ce41f336fdb3
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

can: hi311x: drop hi3110_lock before free_irq() on open failure

hi3110_open() requests a threaded IRQ and then performs hardware setup while
holding priv->hi3110_lock. If setup fails, dropping the lock before calling
free_irq() prevents a deadlock against a pending IRQ thread.

Link: https://patch.msgid.link/20260820020631.316418-1-runyu.xiao@seu.edu.cn

> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index ae90e6716de5b..2be851e8907d0 100644
> --- a/drivers/net/can/spi/hi311x.c
> +++ b/drivers/net/can/spi/hi311x.c
> @@ -787,7 +787,10 @@ static int hi3110_open(struct net_device *net)
>  	return 0;
>  
>   out_free_irq:
> +	priv->force_quit = 1;
> +	mutex_unlock(&priv->hi3110_lock);
>  	free_irq(spi->irq, priv);
> +	mutex_lock(&priv->hi3110_lock);
>  	hi3110_hw_sleep(spi);
>   out_close:
>  	hi3110_power_enable(priv->transceiver, 0);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent execution of
hi3110_clean() without mutual exclusion result in a double-free of
priv->tx_skb and a data race on priv->tx_busy?

It appears hi3110_do_set_mode() can enter hi3110_clean() unlocked, while
hi3110_tx_work_handler() can enter it locked. If they run concurrently, could
this result in simultaneously invoking dev_kfree_skb() and corrupting the
echo skb state?

drivers/net/can/spi/hi311x.c:hi3110_clean() {
    ...
    dev_kfree_skb(priv->tx_skb);
    if (priv->tx_busy)
        can_free_echo_skb(priv->net, 0, NULL);
    ...
}

[Severity: High]
This is a pre-existing issue, but does hi3110_stop() need to cancel pending
driver workqueues like priv->restart_work and priv->tx_work?

Currently, there is no cancel_work_sync() in hi3110_stop(). Could this lead
to hardware access after power off if hi3110_can_remove() disables main VDD
power before the workqueue is drained?

For instance, if hi3110_restart_work_handler() runs after power off, would
it force execution against powered-off hardware and cause SPI bus faults?

drivers/net/can/spi/hi311x.c:hi3110_restart_work_handler() {
    ...
    if (priv->restart_tx) {
        priv->restart_tx = 0;
        hi3110_hw_reset(spi);
        hi3110_setup(net);
        hi3110_clean(net);
        hi3110_set_normal_mode(spi);
        netif_wake_queue(net);
    }
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=14

Re: [PATCH net 11/14] can: kvaser_pciefd: fix use-after-free in bec poll timer

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4173 bytes --]

Fan Wu,

can you comment on this?

regards,
Marc

On 27.08.2026 12:10:53, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing dma_rmb() when reading packets from coherent DMA memory.
> - [High] Incomplete timer teardown and missing state checks in ndo_stop (kvaser_pciefd_stop) causes a polling loop leak, state corruption, and potential spurious netif_rx calls when the interface is brought down.
> --
>
> commit d6148cc43f3a9adbcbc3d9812cbf816bc0ffca1f
> Author: Fan Wu <fanwu01@zju.edu.cn>
>
> can: kvaser_pciefd: fix use-after-free in bec poll timer
>
> This commit addresses a use-after-free by switching to timer_shutdown_sync()
> in kvaser_pciefd_remove() and kvaser_pciefd_teardown_can_ctrls(). It ensures
> the bec_poll_timer callback completes before freeing the CAN device during
> module removal or probe errors.
>
> Link: https://patch.msgid.link/20260818063832.383829-1-fanwu01@zju.edu.cn
>
> > diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> > index d8c9bfb202304..a0597db720864 100644
> > --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> > +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
>
> [ ... ]
>
> > @@ -1879,7 +1880,7 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)
> >  		struct kvaser_pciefd_can *can = pcie->can[i];
> >
> >  		unregister_candev(can->can.dev);
> > -		timer_delete(&can->bec_poll_timer);
> > +		timer_shutdown_sync(&can->bec_poll_timer);
>
> [Severity: High]
> This is a pre-existing issue, but since you are fixing the timer teardown here,
> does the timer also need to be synced during ndo_stop?
>
> In kvaser_pciefd_stop(), the code uses the non-sync timer_delete:
>
> 	iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
> 	timer_delete(&can->bec_poll_timer);
> 	can->can.state = CAN_STATE_STOPPED;
>
> Because this does not wait for the callback to finish, couldn't a pending status
> request be issued to the hardware right as the interface is brought down?
>
> Furthermore, if a status response arrives via the shared PCIe interrupt after
> the interface is stopped, kvaser_pciefd_handle_status_packet() and
> kvaser_pciefd_handle_status_resp() do not seem to check if the device was
> intentionally stopped. They overwrite the state to an active state and
> unconditionally re-arm the timer:
>
> drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_handle_status_packet() {
>     ...
> 	if (can->can.state != CAN_STATE_BUS_OFF &&
> 	    can->can.state != CAN_STATE_ERROR_ACTIVE) {
> 		mod_timer(&can->bec_poll_timer, KVASER_PCIEFD_BEC_POLL_FREQ);
> 	}
>     ...
> }
>
> Could this lead to an infinite polling loop leak and potential spurious
> netif_rx calls on a stopped interface?
>
> [Severity: Critical]
> This is also a pre-existing issue, but I noticed a potential memory ordering
> issue in kvaser_pciefd_read_packet().
>
> When reading from the coherent DMA buffer:
>
> drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_read_packet() {
>     ...
> 	size = le32_to_cpu(buffer[pos++]);
> 	if (!size) {
> 		*start_pos = 0;
> 		return 0;
> 	}
>
> 	p->header[0] = le32_to_cpu(buffer[pos++]);
>     ...
> }
>
> Is a dma_rmb() needed before reading the packet contents?
>
> Without a read memory barrier, weakly ordered CPU architectures might
> speculatively read the header and payload before the size read completes.
> If the hardware is actively updating the buffer, couldn't the CPU see the
> new size but process stale header or payload data that was speculatively
> prefetched?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=11
>
>

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net 02/14] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

[Oliver Hartkopp]

On 27.08.26 14:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Modifying cloned SKB data without unsharing causes data races and potential silent data corruption in shared CAN FD buffers.
> --
> 
> commit cd014a29d1194dd4eff54897a5d2062f2ec32a0c
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
>      
> This patch converts unreliable ARPHRD_CAN type checks to use the robust
> can_get_ml_priv() function. This prevents TUN/TAP devices from spoofing
> their hardware type to bypass CAN device validation.
> 
> Link: https://patch.msgid.link/20260819160822.8256-1-socketcan@hartkopp.net
> 
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 7bc86b176b4de..ef435f22ac933 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -226,7 +226,7 @@ int can_send(struct sk_buff *skb, int loop)
>>   		goto inval_skb;
>>   	}
>>   
>> -	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
>> +	if (unlikely(!can_get_ml_priv(skb->dev))) {
> 
> [Severity: High]
> This is a pre-existing issue, but while reviewing the changes in can_send(),
> I noticed a potential data race when handling cloned SKBs.
> 
> When the CAN gateway forwards a frame without modification functions,
> can_can_gw_rcv() forwards a cloned SKB:
> 
> net/can/gw.c:can_can_gw_rcv() {
>      ...
>      nskb = skb_clone(skb, GFP_ATOMIC);
>      ...
>      can_send(nskb, gwj->limit_hops);
> }
> 
> Then in can_send() just above this diff hunk, the SKB data area is modified
> unconditionally for CAN FD frames:
> 
> net/can/af_can.c:can_send() {
>      ...
>      } else if (can_is_canfd_skb(skb)) {
>          struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>          ...
>          /* set CAN FD flag for CAN FD frames by default */
>          cfd->flags |= CANFD_FDF;
>      }
>      ...
> }
> 
> Since skb_clone() shares the data area with the original SKB, does modifying
> cfd->flags without unsharing the SKB first cause a data race?
> 
> On architectures without atomic byte-store instructions, this performs a
> word-sized read-modify-write operation. If the original SKB is concurrently
> processed by other receivers, could this silently corrupt adjacent fields?
> Given that an unprivileged user with CAP_NET_ADMIN in a user namespace can
> set up a virtual CAN interface and trigger this path, is this safe?

Yes. Sashiko groundhog day ...

https://lore.kernel.org/linux-can/ac22b59e-129b-4883-b9c6-da5e69e4078d@hartkopp.net/


> 
>>   		err = -EPERM;
>>   		goto inval_skb;
>>   	}
> 
> [ ... ]
> 

Re: [PATCH net 11/14] can: kvaser_pciefd: fix use-after-free in bec poll timer

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

On 27.08.2026 14:36:25, Marc Kleine-Budde wrote:
> Fan Wu,
>
> can you comment on this?

Doh, the patch is question is #12, not this one.

Sorry for the noise,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net 12/14] can: kvaser_usb: validate command format before parsing in hydra receive path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5052 bytes --]

Hello Cen Zhang,

On 27.08.2026 12:10:55, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] The patch prevents a specific out-of-bounds read of `cmd->len` by verifying at least 6 bytes are available, but it fails to enforce a safe minimum structural length for extended commands. A malicious USB device can provide a trivially small `cmd->len` (e.g., 8), bypassing the buffer boundary checks in the parser and leading to severe slab-out-of-bounds reads in downstream command handlers.

can you comment on this one?

regards,
Marc

>
> Pre-existing issues:
> - [Critical] Missing validation of the `echo_index` sentinel value allows a malicious USB device to trigger a slab-out-of-bounds read and an arbitrary kernel pointer write, leading to potential privilege escalation.
> --
>
> commit 36da6d5bcb0fd8b842d165066e47c1ff8b40c308
> Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>
>
> can: kvaser_usb: validate command format before parsing in hydra receive path
>
> This commit adds bounds checking for command formats in the kvaser_usb hydra
> receive path. It aims to prevent a slab-out-of-bounds read caused by malicious
> USB devices sending truncated headers or zero-length commands.
>
> Link: https://patch.msgid.link/20260826013037.6933-1-blbllhy@gmail.com
>
> > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> > index efbb7bed34c9d..efe7b3ef489bf 100644
> > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> > @@ -536,6 +536,24 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
> >  	return ret;
> >  }
> >
> > +/* -EAGAIN means incomplete; -EINVAL rejects zero to ensure progress. */
> > +static int kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
> > +					size_t remaining, size_t *cmd_len)
> > +{
> > +	if (remaining < sizeof(cmd->header.cmd_no))
> > +		return -EAGAIN;
> > +
> > +	if (cmd->header.cmd_no == CMD_EXTENDED &&
> > +	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
> > +		return -EAGAIN;
> > +
> > +	*cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> > +	if (!*cmd_len)
> > +		return -EINVAL;
>
> [Severity: High]
> Does this length validation allow maliciously small extended commands?
>
> While this prevents cmd_len from being 0, it allows trivially small values
> (e.g., 8 bytes). If a malicious device places an 8-byte extended command at
> the very end of a 3072-byte USB bulk transfer, kvaser_usb_hydra_cmd_size_rx()
> will return 0 and set cmd_len to 8. Because pos + 8 == len, the overarching
> boundary check in the parser will pass, handing the 8-byte command to
> kvaser_usb_hydra_handle_cmd().
>
> Downstream handlers like kvaser_usb_hydra_rx_msg_ext() then blindly access
> payload fields extending far beyond 8 bytes (such as cmd->rx_can.kcan_header
> at offset 20). Does this code read out-of-bounds of the 3072-byte heap
> allocation?
>
> > +
> > +	return 0;
> > +}
> > +
> >  static struct kvaser_usb_net_priv *
> >  kvaser_usb_hydra_net_priv_from_cmd(const struct kvaser_usb *dev,
> >  				   const struct kvaser_cmd *cmd)
>
> [ ... ]
>
> [Severity: Critical]
> This isn't a bug introduced by this patch, but while reviewing this file I
> noticed a pre-existing issue where an unvalidated echo_index can lead to
> an out-of-bounds read and arbitrary memory corruption.
>
> In kvaser_usb_hydra_tx_acknowledge():
>
> 	u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
> 	...
> 	context = &priv->tx_contexts[transid % dev->max_tx_urbs];
> 	...
> 	skb = priv->can.echo_skb[context->echo_index];
> 	if (skb)
> 		skb_hwtstamps(skb)->hwtstamp = kvaser_usb_hydra_ktime_from_cmd(dev->cfg, cmd);
>
> When a TX context is free (or newly initialized), context->echo_index is
> set to the sentinel value dev->max_tx_urbs:
>
> 	context->echo_index = dev->max_tx_urbs;
>
> If a malicious USB device sends a spoofed CMD_TX_ACKNOWLEDGE with a transid
> matching a free TX context, this code directly indexes priv->can.echo_skb
> using dev->max_tx_urbs.
>
> Since echo_skb is placed exactly at the end of the net_device memory block
> (via alloc_candev_mqs), an index of max_tx_urbs reads 8 bytes of garbage
> from the subsequent kernel slab object.
>
> If the read value is non-NULL, the driver dereferences it and writes a
> 64-bit timestamp derived from the attacker-controlled USB packet into memory.
> Can this be triggered by a malicious USB device?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=12
>
>

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net 10/14] can: dev: can_put_echo_skb(): free skb on invalid echo index

[Oliver Hartkopp]

On 27.08.26 14:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Modifying SKB payload in can_skb_init_valid() without unsharing causes data corruption for concurrent observers.
> --
> 
> commit 6a48fe2d96b6f1ca706da34b2f7bdd872d06941f
> Author: Cunhao Lu <1579567540@qq.com>
> can: dev: can_put_echo_skb(): free skb on invalid echo index
> 
> This patch fixes a potential leak in can_put_echo_skb() when the echo index
> is out of bounds. It ensures the socket buffer is freed before returning
> -EINVAL so that all error paths correctly consume the SKB.
> 
> Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com
> 
>> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
>> index c7be21ed70ddf..faafb1033d423 100644
>> --- a/drivers/net/can/dev/skb.c
>> +++ b/drivers/net/can/dev/skb.c
> 
> [ ... ]
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but already existed in the
> codebase. Does can_skb_init_valid() in this same file modify shared memory
> without unsharing it first?
> 
> drivers/net/can/dev/skb.c:can_skb_init_valid() {
>          ...
>          /* set CANFD_FDF flag for CAN FD frames */
>          if (can_is_canfd_skb(skb)) {
>              struct canfd_frame *cfd;
> 
>              cfd = (struct canfd_frame *)skb->data;
>              cfd->flags |= CANFD_FDF;
>          }
>          ...
> }
> 
> If a user with CAP_NET_RAW injects a CAN FD frame via an AF_PACKET socket,
> the packet goes to dev_queue_xmit() where it may be cloned for packet
> sniffers like tcpdump.
> 
> When the original SKB reaches the driver's transmit function and calls
> can_dev_dropped_skb() -> can_dropped_invalid_skb() -> can_skb_init_valid(),
> it modifies the payload (cfd->flags |= CANFD_FDF) without verifying
> skb_cloned(skb) or calling skb_unshare().
> 
> Can this cause data corruption for concurrent observers receiving the clones?
> 

No.

https://lore.kernel.org/linux-can/ac22b59e-129b-4883-b9c6-da5e69e4078d@hartkopp.net/

Re: [PATCH net 03/14] can: bittiming: fix divide-by-zero in can_calc_bittiming()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: bittiming: fix divide-by-zero in can_calc_bittiming()

can_calc_bittiming() computed the prescaler as priv->clock.freq /
(tsegall * bt->bitrate), where the product is a 32 bit multiplication that
wraps to zero for large userspace supplied bitrates. The fix computes the
product with mul_u32_u32() and divides with div64_u64().

Link: https://patch.msgid.link/20260803091426.29050-2-ssureshmsd7@gmail.com

> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 42498e9d3f38d..4809f5e0c96e9 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c
> @@ -119,8 +119,12 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  	     tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
>  		tsegall = CAN_SYNC_SEG + tseg / 2;
>  
> -		/* Compute all possible tseg choices (tseg=tseg1+tseg2) */
> -		brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
> +		/* Compute all possible tseg choices (tseg=tseg1+tseg2).
> +		 * A 32 bit tsegall * bt->bitrate can wrap to zero for large
> +		 * userspace bitrates, so compute the product in 64 bit.
> +		 */
> +		brp = div64_u64(priv->clock.freq,
> +				mul_u32_u32(tsegall, bt->bitrate)) + tseg % 2;
>  
>  		/* choose brp step which is possible in system */
>  		brp = (brp / btc->brp_inc) * btc->brp_inc;

[Severity: High]

This is a pre-existing issue and not something introduced by this patch, but
can the same input class the commit message describes still end in a divide
error two statements later?

With the 64 bit product, a bitrate large enough that clock.freq is smaller
than tsegall * bt->bitrate now yields brp == 0 for even tseg (tseg % 2 == 0)
instead of faulting in the patched expression. The only thing that stops
brp == 0 from reaching the next division is the range guard:

drivers/net/can/dev/calc_bittiming.c:can_calc_bittiming() {
	...
	brp = (brp / btc->brp_inc) * btc->brp_inc;
	if (brp < btc->brp_min || brp > btc->brp_max)
		continue;

	bitrate = priv->clock.freq / (brp * tsegall);
	...
}

so it relies on btc->brp_min being at least 1, and the first of those two
statements relies on btc->brp_inc being non-zero.

Are those two fields validated anywhere in the core? The only core
validator of struct can_bittiming_const, can_bittiming_const_valid() in
drivers/net/can/dev/dev.c called from register_candev(), only rejects a
zero sjw_max:

drivers/net/can/dev/dev.c:can_bittiming_const_valid() {
	if (!btc)
		return true;

	if (!btc->sjw_max)
		return false;

	return true;
}

All statically initialised in-tree constants use brp_min = 1 and
brp_inc >= 1, but gs_make_candev() in drivers/net/can/usb/gs_usb.c fills
the struct from a device reply without any range check:

	dev->bt_const.brp_min = le32_to_cpu(bt_const.brp_min);
	dev->bt_const.brp_max = le32_to_cpu(bt_const.brp_max);
	dev->bt_const.brp_inc = le32_to_cpu(bt_const.brp_inc);

and the identical copy of bt_const_extended.dbrp_* into
dev->data_bt_const for the CAN FD data const. Both are then handed to
can_calc_bittiming() through can_changelink() -> can_get_bittiming(),
which is entered when bt->bitrate is set and bt->tq is 0.

For a device reporting brp_inc == 0, does brp / btc->brp_inc fault on the
first loop iteration for any bitrate? And for a device reporting
brp_min == 0 with a small fclk_can, does the freq / (brp * tsegall)
division fault with brp == 0 for the same wrapping-class bitrate this
patch targets?

Since can_changelink() runs with rtnl_lock held, would the resulting oops
leave rtnl_lock held permanently? Would it make sense to extend
can_bittiming_const_valid() to also reject brp_inc == 0 and brp_min == 0,
or to add a brp check before those divisions?

Re: [PATCH net 06/14] can: rockchip_canfd: retry the outstanding TX buffer

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: rockchip_canfd: retry the outstanding TX buffer

This changes rkcanfd_xmit_retry() to use rkcanfd_get_tx_tail() instead of
rkcanfd_get_tx_head() when building the TX_REQ command, so that the
erratum 6 workaround re-requests the buffer that is actually outstanding
now that the TX FIFO depth is two.

Link: https://patch.msgid.link/tencent_2CCE005FC440BF8026599A6999EC04BE1107@qq.com

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 86fa8f2e1c8ba..fc338ea865feb 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> @@ -57,8 +57,8 @@ static void rkcanfd_start_xmit_write_cmd(const struct rkcanfd_priv *priv,
>  
>  void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
>  {
> -	const unsigned int tx_head = rkcanfd_get_tx_head(priv);
> -	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_head);
> +	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
> +	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
>  }

[Severity: Medium]

This isn't a bug introduced by this patch, but with rkcanfd_xmit_retry()
now aiming at the tx_tail buffer, is the buffer it re-requests the same
one that rkcanfd_start_xmit() may be arming at that moment?

rkcanfd_xmit_retry() is reached from the hard-IRQ handler, since
rkcanfd_open() registers it non-threaded:

	err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);

and rkcanfd_irq()->rkcanfd_handle_rx_int()->rkcanfd_rxstx_filter() calls
rkcanfd_xmit_retry() for the erratum 6 case. At this commit there is no
lock covering priv->tx_head, priv->tx_tail or the register writes.

Two interleavings look possible.

First, rkcanfd_start_xmit_write_cmd() is not atomic:

	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
		rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default |
			      RKCANFD_REG_MODE_SPACE_RX_MODE);

	rkcanfd_write(priv, RKCANFD_REG_CMD, reg_cmd);

	if (priv->devtype_data.quirks & RKCANFD_QUIRK_RK3568_ERRATUM_12)
		rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);

If the interrupt path runs this sequence while the transmit path is
between its own MODE and CMD writes, the restore of reg_mode_default
lands before the other context's CMD write. Does that silently drop the
erratum 12 workaround for that frame? rk3568v2 carries both
RKCANFD_QUIRK_RK3568_ERRATUM_6 and RKCANFD_QUIRK_RK3568_ERRATUM_12.

Second, rkcanfd_start_xmit() publishes the new head before issuing the
command:

	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);

	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);

If the interrupt path completes the previous frame (advancing tx_tail)
in that window, rkcanfd_xmit_retry() now selects the buffer whose CMD
write has not been issued yet. Can that produce a second TX request for
the same buffer, a duplicated frame on the wire, and a desynchronized
echo ring, with rkcanfd_tx_tail_is_eff() then hitting the

	"%s: echo_skb[%u]=NULL tx_head=0x%08x tx_tail=0x%08x\n"

path?

For what it is worth, this appears to be addressed later in the same
series by the change that adds "spinlock_t tx_lock; /* protects tx_head,
tx_tail and echo_skb */" to struct rkcanfd_priv, wraps the
rkcanfd_start_xmit() echo/register/command sequence in
spin_lock_irqsave()/spin_unlock_irqrestore(), takes the same lock in
rkcanfd_rxstx_filter() and adds lockdep_assert_held(&priv->tx_lock) to
rkcanfd_xmit_retry(). Given that ordering, would it be worth noting the
dependency in this patch so that a standalone stable backport of this
change does not land without the locking?

Re: [PATCH net 07/14] can: rockchip_canfd: serialize TX state and command writes

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: rockchip_canfd: serialize TX state and command writes

This adds a new spinlock priv->tx_lock to the rockchip_canfd driver and uses
it to serialize tx_head, tx_tail and the echo skb ring between the transmit
path and the RX/completion path.  The same lock is also taken around the
erratum 12 MODE/CMD/MODE register sequence, and lockdep_assert_held() is
added to the helpers that now require it.

Link: https://patch.msgid.link/tencent_AF224EFBC9343DD238C80824AC8CA805480A@qq.com

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 24e87daa1df00..59420233c9184 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> @@ -100,14 +100,25 @@ static int rkcanfd_rxstx_filter(struct rkcanfd_priv *priv,
>  	const struct canfd_frame *cfd_nominal;
>  	const struct sk_buff *skb;
>  	unsigned int tx_tail;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +
> +	if (!rkcanfd_get_tx_pending(priv))
> +		goto out_unlock;
>  
>  	tx_tail = rkcanfd_get_tx_tail(priv);
>  	skb = priv->can.echo_skb[tx_tail];
>  	if (!skb) {
> +		const unsigned int tx_head_unmasked = priv->tx_head;
> +		const unsigned int tx_tail_unmasked = priv->tx_tail;
> +
> +		spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
>  		netdev_err(priv->ndev,
>  			   "%s: echo_skb[%u]=NULL tx_head=0x%08x tx_tail=0x%08x\n",
>  			   __func__, tx_tail,
> -			   priv->tx_head, priv->tx_tail);
> +			   tx_head_unmasked, tx_tail_unmasked);
>  
>  		return -ENOMSG;
>  	}

[ ... ]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index fc338ea865feb..b367341dd0aeb 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c

[ ... ]

> @@ -60,6 +71,8 @@ void rkcanfd_xmit_retry(struct rkcanfd_priv *priv)
>  	const unsigned int tx_tail = rkcanfd_get_tx_tail(priv);
>  	const u32 reg_cmd = RKCANFD_REG_CMD_TX_REQ(tx_tail);
>  
> +	lockdep_assert_held(&priv->tx_lock);
> +
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
>  }

[ ... ]

> @@ -88,10 +102,13 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  		return NETDEV_TX_BUSY;
>  	}
>  
> +	spin_lock_irqsave(&priv->tx_lock, flags);
>  	tx_head = rkcanfd_get_tx_head(priv);
>  	frame_len = can_skb_get_frame_len(skb);
>  	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
>  	if (err) {
> +		spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
>  		ndev->stats.tx_dropped++;
>  		return NETDEV_TX_OK;
>  	}

[Severity: Low]
The last paragraph of the changelog says:

    "Install the echo skb before loading the hardware TX buffer so
     an echo setup failure cannot desynchronize the hardware and software TX
     state."

Is that part of this change?  In the hunk above, the can_put_echo_skb()
call and its error return are unmodified context; only the two
spin_lock_irqsave()/spin_unlock_irqrestore() lines are added.  That
reordering came from the preceding commit 4a223c132464 ("can:
rockchip_canfd: prevent TX stall on echo skb failure"), which itself
depends on the can_put_echo_skb() ownership change that landed later in the
same series as "can: dev: can_put_echo_skb(): free skb on invalid echo
index".

The same applies to rkcanfd_xmit_retry() above: selecting the retry slot
with tx_tail instead of tx_head comes from 0b345ad5c3d3 ("can:
rockchip_canfd: retry the outstanding TX buffer"), and this patch only adds
lockdep_assert_held() there.

Since this commit carries Cc: stable and two Fixes: tags, could the message
name those prerequisites so a backport into a tree without 4a223c132464 and
0b345ad5c3d3 is not attempted as a standalone cherry-pick?

> @@ -136,6 +153,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);

[Severity: High]
Does tx_lock really make the erratum 12 sequence atomic against all MODE
writers?  The protected sequence is:

  rkcanfd_start_xmit_write_cmd()
      rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default | RKCANFD_REG_MODE_SPACE_RX_MODE);
      rkcanfd_write(priv, RKCANFD_REG_CMD, reg_cmd);
      rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);

Two other RKCANFD_REG_MODE writers in rockchip_canfd-core.c do not take
tx_lock:

  rkcanfd_chip_set_reset_mode()
      reset_control_assert(priv->reset);
      udelay(2);
      reset_control_deassert(priv->reset);
      rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);

  rkcanfd_chip_set_work_mode()
      rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);

rkcanfd_chip_set_reset_mode() is reachable from hard interrupt context:

  rkcanfd_irq() -> rkcanfd_handle_state_error_int() ->
    rkcanfd_chip_stop(priv, CAN_STATE_BUS_OFF) -> __rkcanfd_chip_stop() ->
      rkcanfd_chip_set_reset_mode()

spin_lock_irqsave() masks interrupts only on the local CPU, so on SMP that
handler can run on another CPU between the three writes above.  If
MODE = 0x0 lands between the SPACE_RX_MODE write and the CMD write, does CMD
not then execute with SPACE_RX_MODE cleared, which is exactly the erratum 12
bypass that Fixes: 83f9bd6bf39d is meant to close?

If the reset lands just before the trailing restore instead, the transmit
path writes MODE = priv->reg_mode_default (which includes
RKCANFD_REG_MODE_WORK_MODE) after the bus-off handler reset the controller
and after rkcanfd_chip_interrupts_disable() masked all interrupts.  Can that
leave the hardware running in work mode while priv->can.state is
CAN_STATE_BUS_OFF, with no interrupt left to recover the interface?

Can the two paths really overlap?  can_bus_off() only calls
netif_carrier_off() and schedules restart_work, and the TX queue is
deactivated asynchronously through linkwatch, so a frame already queued in
the qdisc can still enter rkcanfd_start_xmit() on another CPU while the
bus-off interrupt resets the chip.

Would it make sense to have these MODE writers take tx_lock as well, or to
document why they are exempt?

[ ... ]

> diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
> index 95bea9bfd8a2f..2dc3a41afe32d 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd.h
> +++ b/drivers/net/can/rockchip/rockchip_canfd.h

[ ... ]

> @@ -474,6 +475,7 @@ struct rkcanfd_priv {
>  	struct can_rx_offload offload;
>  	struct net_device *ndev;
>  
> +	spinlock_t tx_lock; /* protects tx_head, tx_tail and echo_skb */
>  	void __iomem *regs;
>  	unsigned int tx_head;
>  	unsigned int tx_tail;

[Severity: Medium]
The new comment states that tx_lock protects tx_head, tx_tail and echo_skb,
and the patch enforces that with lockdep_assert_held() in
rkcanfd_tx_tail_is_eff(), rkcanfd_xmit_retry() and
rkcanfd_handle_tx_done_one().  Should rkcanfd_chip_fifo_setup() in
rockchip_canfd-core.c take the lock too?  It writes both counters with no
lock held:

	WRITE_ONCE(priv->tx_head, 0);
	WRITE_ONCE(priv->tx_tail, 0);
	netdev_reset_queue(priv->ndev);

It runs on the bus-off restart and "ip link ... type can restart" path:

  can_restart() -> netif_carrier_on() -> priv->do_set_mode(CAN_MODE_START)
    -> rkcanfd_set_mode() -> rkcanfd_chip_start() -> rkcanfd_chip_fifo_setup()

can_restart() turns the carrier on before calling do_set_mode(), and carrier
changes only queue linkwatch work, so dev_activate() can re-arm the qdisc
while rkcanfd_chip_fifo_setup() is still zeroing the counters.

Can this interleave?

CPU0 rkcanfd_start_xmit() (holds tx_lock)
    tx_head = rkcanfd_get_tx_head();          /* N */
    can_put_echo_skb(skb, ndev, tx_head, frame_len);
    ... hardware TX buffer writes ...
    WRITE_ONCE(priv->tx_head, N + 1);

CPU1 rkcanfd_chip_fifo_setup()
    WRITE_ONCE(priv->tx_head, 0);
    WRITE_ONCE(priv->tx_tail, 0);
    netdev_reset_queue(priv->ndev);

Afterwards the head/tail pair describes neither the echo ring nor the
hardware FIFO, which looks like it can produce the "echo_skb[%u]=NULL"
errors and a stopped TX queue with no completion left to wake it.  The same
restart path also frees echo skbs through can_flush_echo_skb() without
tx_lock, and netdev_reset_queue() there races netdev_sent_queue() in the
transmit path.

Two smaller inconsistencies with the documented rule: the diagnostic in
rkcanfd_rxstx_filter() is now taken from a locked snapshot, while the
identical one in rkcanfd_start_xmit() still reads both fields unlocked:

		netdev_info(priv->ndev,
			    "Stopping tx-queue (tx_head=0x%08x, tx_tail=0x%08x, tx_pending=%d)\n",
			    priv->tx_head, priv->tx_tail,
			    rkcanfd_get_tx_pending(priv));

and rkcanfd_handle_rx_int_one() still gates entry into the now
lock-protected rkcanfd_rxstx_filter() on an unlocked
rkcanfd_get_tx_pending().  Were both left out on purpose?

Re: [PATCH net 12/14] can: kvaser_usb: validate command format before parsing in hydra receive path

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: kvaser_usb: validate command format before parsing in hydra receive path

This adds kvaser_usb_hydra_cmd_size_rx() so the hydra receive paths check
that the CMD_EXTENDED 2-byte length field is fully present before reading
it, and rejects a declared length of zero to keep the parse loop making
progress.  The asynchronous path now keeps a partial extended header in the
leftover buffer and completes it from the next transfer.

Link: https://patch.msgid.link/20260826013037.6933-1-blbllhy@gmail.com

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> index efbb7bed34c9d..efe7b3ef489bf 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -536,6 +536,24 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
>  	return ret;
>  }
>  
> +/* -EAGAIN means incomplete; -EINVAL rejects zero to ensure progress. */
> +static int kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
> +					size_t remaining, size_t *cmd_len)
> +{
> +	if (remaining < sizeof(cmd->header.cmd_no))
> +		return -EAGAIN;
> +
> +	if (cmd->header.cmd_no == CMD_EXTENDED &&
> +	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
> +		return -EAGAIN;
> +
> +	*cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +	if (!*cmd_len)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: High]
Should this helper also reject a declared length that is smaller than the
fields the extended-command consumers dereference?

Right now the only constraint on the device-supplied length is that it is
non-zero, but struct kvaser_cmd_ext is 96 bytes with cmd_no_ext at offset 6,
rx_can.flags at 8, kcan_header at 20, timestamp at 24 and kcan_payload at
32..95:

	struct kvaser_cmd_ext {
		struct kvaser_cmd_header header;
		__le16 len;
		u8 cmd_no_ext;
		u8 reserved;
		union {
			struct kvaser_cmd_ext_rx_can rx_can;
			...
	} __packed;

kvaser_usb_hydra_handle_cmd_ext() reads cmd->cmd_no_ext and then
kvaser_usb_hydra_rx_msg_ext() reads all the way up to kcan_payload,
without any relation to cmd->len:

	drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_rx_msg_ext() {
		...
		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
		...
	}

The sibling leaf driver already enforces per-command minimums before
dispatch via kvaser_usb_leaf_verify_size() in
kvaser_usb_leaf_handle_command().  Would it make sense to require
cmd_len >= offsetof(struct kvaser_cmd_ext, cmd_no_ext) + 2 here, or better a
per-cmd_no_ext minimum up to sizeof(struct kvaser_cmd_ext), instead of only
rejecting zero?

> @@ -675,8 +693,8 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
>  			size_t cmd_len;
>  
>  			tmp_cmd = buf + pos;
> -			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
> -			if (pos + cmd_len > actual_len) {
> +			err = kvaser_usb_hydra_cmd_size_rx(tmp_cmd, actual_len - pos, &cmd_len);
> +			if (err || pos + cmd_len > actual_len) {
>  				dev_err_ratelimited(&dev->intf->dev,
>  						    "Format error\n");
>  				break;

[Severity: High]
In kvaser_usb_hydra_wait_cmd(), a CMD_EXTENDED declaring a small positive
length (say 1) still returns 0 from the helper, and cmd_len is then used as
the stride:

			pos += cmd_len;

Does this leave the parser resynchronising from the middle of the malformed
extended header for the rest of the buffer?  The accesses stay inside the
kzalloc'd buffer here, but the interpretation of the following bytes is
attacker-chosen.

> @@ -2120,23 +2138,47 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	spin_lock_irqsave(usb_rx_leftover_lock, irq_flags);
>  	usb_rx_leftover_len = card_data->usb_rx_leftover_len;
>  	if (usb_rx_leftover_len) {
> +		const size_t cmd_size_field_end = offsetof(struct kvaser_cmd_ext, cmd_no_ext);
>  		int remaining_bytes;
> +		int err;
>  
>  		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		if (cmd->header.cmd_no == CMD_EXTENDED &&
> +		    usb_rx_leftover_len < cmd_size_field_end) {
> +			remaining_bytes = min_t(int, len, cmd_size_field_end - usb_rx_leftover_len);
> +
> +			memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf, remaining_bytes);
> +			usb_rx_leftover_len += remaining_bytes;
> +			card_data->usb_rx_leftover_len = usb_rx_leftover_len;
> +			pos += remaining_bytes;
>  
> -		remaining_bytes = min_t(unsigned int, len,
> +			if (usb_rx_leftover_len < cmd_size_field_end) {
> +				spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
> +				return;
> +			}
> +		}
> +
> +		err = kvaser_usb_hydra_cmd_size_rx(cmd, usb_rx_leftover_len, &cmd_len);
> +		if (err || cmd_len < usb_rx_leftover_len) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			card_data->usb_rx_leftover_len = 0;
> +			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
> +			return;
> +		}
> +
> +		remaining_bytes = min_t(unsigned int, len - pos,
>  					cmd_len - usb_rx_leftover_len);

[Severity: High]
On the reassembly path, once the top-up above has filled exactly the 6 bytes
up to cmd_size_field_end, a device-declared cmd_len of 6, 7 or 8 passes the
helper and then satisfies the completion test straight away:

		if (remaining_bytes + usb_rx_leftover_len == cmd_len) {
			kvaser_usb_hydra_handle_cmd(dev, cmd);

Does this dispatch an extended command whose cmd_no_ext and payload bytes
come from whatever was previously left in the 128-byte
card_data->usb_rx_leftover array, since only the first 6 bytes have been
written by this transfer?

> @@ -2152,11 +2194,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
>  
>  	while (pos < len) {
> +		int err;
> +
>  		cmd = buf + pos;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		err = kvaser_usb_hydra_cmd_size_rx(cmd, len - pos, &cmd_len);
> +		if (err && err != -EAGAIN) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}
>  
> -		if (pos + cmd_len > len) {
> +		if (err == -EAGAIN || pos + cmd_len > len) {
>  			/* We got first part of a command */
>  			int leftover_bytes;
>  

[Severity: High]
Can a crafted transfer still reach the same class of out-of-bounds read that
this commit describes?

Here err == 0 together with pos + cmd_len <= len is the complete gate before
kvaser_usb_hydra_handle_cmd(), and pos is advanced by the device's own
declared lengths, so pos can be placed anywhere in the 3072-byte URB buffer
allocated in kvaser_usb_setup_rx_urbs():

	buf = usb_alloc_coherent(dev->udev, KVASER_USB_RX_BUFFER_SIZE,
				 GFP_KERNEL, &buf_dma);

For a full 3072-byte transfer holding a CMD_EXTENDED with len 3064, followed
at pos 3064 by a CMD_EXTENDED with len 8 and cmd_no_ext CMD_RX_MESSAGE_FD:

  remaining is 8, which is >= offsetof(struct kvaser_cmd_ext, cmd_no_ext)
  cmd_len is 8, so the zero check passes
  pos + cmd_len == 3072 == len, so the truncation check passes

kvaser_usb_hydra_handle_cmd() -> kvaser_usb_hydra_handle_cmd_ext() ->
kvaser_usb_hydra_rx_msg_ext() then reads rx_can.flags, kcan_header and
timestamp past the end of the transfer buffer and finishes with:

		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

with cf->len up to 64 taken from the device-supplied DLC, i.e. up to roughly
88 bytes beyond the 3072-byte allocation.  On host controllers where
hcd_uses_dma() is false (dummy_hcd, vhci) hcd_buffer_alloc() falls back to
kmalloc(3072), so KASAN would report a slab-out-of-bounds read here, and the
bytes read are then handed to netif_rx() inside a CAN FD frame.

Would adding the minimum-length check inside kvaser_usb_hydra_cmd_size_rx()
close this as well, rather than leaving it for a follow-up?