Skip to contentSkip to conversation

can: kvaser_usb: Various fixes

PATCH In review Jimmy Assarsson 2 participants 6 messages Oct 10, 2022 – Oct 11, 2022

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v5 cover can: kvaser_usb: Various fixes

Changes in v5:
- Split series [1], keept only critical bug fixes that should go into stable, since v4 got rejected [2].
Non-critical fixes are posted in a separate series.

[1]
https://lore.kernel.org/linux-can/[email protected]
[2]
https://lore.kernel.org/linux-can/[email protected]
Anssi Hannula (4):
can: kvaser_usb_leaf: Fix overread with an invalid command
can: kvaser_usb: Fix use of uninitialized completion
can: kvaser_usb_leaf: Fix TX queue out of sync after restart
can: kvaser_usb_leaf: Fix CAN state after restart

 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  2 +
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |  3 +-
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c |  2 +-
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 79 +++++++++++++++++++
 4 files changed, 84 insertions(+), 2 deletions(-)
Signature · 1 line
2.38.0
#1 lore raw

PATCH v5 1/4 can: kvaser_usb_leaf: Fix overread with an invalid command

From: Anssi Hannula <[email protected]>

For command events read from the device, kvaser_usb_leaf_read_bulk_callback() verifies that cmd->len does not exceed the size of the received data, but the actual kvaser_cmd handlers will happily read any kvaser_cmd fields without checking for cmd->len.

This can cause an overread if the last cmd in the buffer is shorter than expected for the command type (with cmd->len showing the actual short size).

Maximum overread seems to be 22 bytes (CMD_LEAF_LOG_MESSAGE), some of which are delivered to userspace as-is.

Fix that by verifying the length of command before handling it.

This issue can only occur after RX URBs have been set up, i.e. the interface has been opened at least once.

Fixes 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")

---
Changes in v5:
- Split series and rebased

Changes in v4:
- No changes

Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b

Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")

 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +75 -0
index 07f687f29b34..8e11cda85624 100644
@@ -310,6 +310,38 @@ struct kvaser_cmd {
 	} u;
 } __packed;
 
+#define CMD_SIZE_ANY 0xff
+#define kvaser_fsize(field) sizeof_field(struct kvaser_cmd, field)
+
+static const u8 kvaser_usb_leaf_cmd_sizes_leaf[] = {
+	[CMD_START_CHIP_REPLY]		= kvaser_fsize(u.simple),
+	[CMD_STOP_CHIP_REPLY]		= kvaser_fsize(u.simple),
+	[CMD_GET_CARD_INFO_REPLY]	= kvaser_fsize(u.cardinfo),
+	[CMD_TX_ACKNOWLEDGE]		= kvaser_fsize(u.tx_acknowledge_header),
+	[CMD_GET_SOFTWARE_INFO_REPLY]	= kvaser_fsize(u.leaf.softinfo),
+	[CMD_RX_STD_MESSAGE]		= kvaser_fsize(u.leaf.rx_can),
+	[CMD_RX_EXT_MESSAGE]		= kvaser_fsize(u.leaf.rx_can),
+	[CMD_LEAF_LOG_MESSAGE]		= kvaser_fsize(u.leaf.log_message),
+	[CMD_CHIP_STATE_EVENT]		= kvaser_fsize(u.leaf.chip_state_event),
+	[CMD_CAN_ERROR_EVENT]		= kvaser_fsize(u.leaf.error_event),
+	/* ignored events: */
+	[CMD_FLUSH_QUEUE_REPLY]		= CMD_SIZE_ANY,
+};
+
+static const u8 kvaser_usb_leaf_cmd_sizes_usbcan[] = {
+	[CMD_START_CHIP_REPLY]		= kvaser_fsize(u.simple),
+	[CMD_STOP_CHIP_REPLY]		= kvaser_fsize(u.simple),
+	[CMD_GET_CARD_INFO_REPLY]	= kvaser_fsize(u.cardinfo),
+	[CMD_TX_ACKNOWLEDGE]		= kvaser_fsize(u.tx_acknowledge_header),
+	[CMD_GET_SOFTWARE_INFO_REPLY]	= kvaser_fsize(u.usbcan.softinfo),
+	[CMD_RX_STD_MESSAGE]		= kvaser_fsize(u.usbcan.rx_can),
+	[CMD_RX_EXT_MESSAGE]		= kvaser_fsize(u.usbcan.rx_can),
+	[CMD_CHIP_STATE_EVENT]		= kvaser_fsize(u.usbcan.chip_state_event),
+	[CMD_CAN_ERROR_EVENT]		= kvaser_fsize(u.usbcan.error_event),
+	/* ignored events: */
+	[CMD_USBCAN_CLOCK_OVERFLOW_EVENT] = CMD_SIZE_ANY,
+};
+
 /* Summary of a kvaser error event, for a unified Leaf/Usbcan error
  * handling. Some discrepancies between the two families exist:
  *
@@ -397,6 +429,43 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_imx_dev_cfg_32mhz = {
 	.bittiming_const = &kvaser_usb_flexc_bittiming_const,
 };
 
+static int kvaser_usb_leaf_verify_size(const struct kvaser_usb *dev,
+				       const struct kvaser_cmd *cmd)
+{
+	/* buffer size >= cmd->len ensured by caller */
+	u8 min_size = 0;
+
+	switch (dev->driver_info->family) {
+	case KVASER_LEAF:
+		if (cmd->id < ARRAY_SIZE(kvaser_usb_leaf_cmd_sizes_leaf))
+			min_size = kvaser_usb_leaf_cmd_sizes_leaf[cmd->id];
+		break;
+	case KVASER_USBCAN:
+		if (cmd->id < ARRAY_SIZE(kvaser_usb_leaf_cmd_sizes_usbcan))
+			min_size = kvaser_usb_leaf_cmd_sizes_usbcan[cmd->id];
+		break;
+	}
+
+	if (min_size == CMD_SIZE_ANY)
+		return 0;
+
+	if (min_size) {
+		min_size += CMD_HEADER_LEN;
+		if (cmd->len >= min_size)
+			return 0;
+
+		dev_err_ratelimited(&dev->intf->dev,
+				    "Received command %u too short (size %u, needed %u)",
+				    cmd->id, cmd->len, min_size);
+		return -EIO;
+	}
+
+	dev_warn_ratelimited(&dev->intf->dev,
+			     "Unhandled command (%d, size %d)\n",
+			     cmd->id, cmd->len);
+	return -EINVAL;
+}
+
 static void *
 kvaser_usb_leaf_frame_to_cmd(const struct kvaser_usb_net_priv *priv,
 			     const struct sk_buff *skb, int *cmd_len,
@@ -502,6 +571,9 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id,
 end:
 	kfree(buf);
 
+	if (err == 0)
+		err = kvaser_usb_leaf_verify_size(dev, cmd);
+
 	return err;
 }
 
@@ -1133,6 +1205,9 @@ static void kvaser_usb_leaf_stop_chip_reply(const struct kvaser_usb *dev,
 static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,
 					   const struct kvaser_cmd *cmd)
 {
+	if (kvaser_usb_leaf_verify_size(dev, cmd) < 0)
+		return;
+
 	switch (cmd->id) {
 	case CMD_START_CHIP_REPLY:
 		kvaser_usb_leaf_start_chip_reply(dev, cmd);
Signature · 1 line
2.38.0
#2 lore raw

PATCH v5 2/4 can: kvaser_usb: Fix use of uninitialized completion

In reply to Jimmy Assarsson

From: Anssi Hannula <[email protected]>

flush_comp is initialized when CMD_FLUSH_QUEUE is sent to the device and completed when the device sends CMD_FLUSH_QUEUE_RESP.

This causes completion of uninitialized completion if the device sends CMD_FLUSH_QUEUE_RESP before CMD_FLUSH_QUEUE is ever sent (e.g. as a response to a flush by a previously bound driver, or a misbehaving device).

Fix that by initializing flush_comp in kvaser_usb_init_one() like the other completions.

This issue is only triggerable after RX URBs have been set up, i.e. the interface has been opened at least once.

Fixes aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")

---
Changes in v5:
- Split series and rebased

Changes in v4:
- No changes

Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b

Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")

 drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c  | 1 +
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +1 -0
index 824cab80aa02..c2bce6773adc 100644
@@ -729,6 +729,7 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev, int channel)
 	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
+	init_completion(&priv->flush_comp);
 	priv->can.ctrlmode_supported = 0;
 
 	priv->dev = dev;
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +1 -1
index 6871d474dabf..7b52fda73d82 100644
@@ -1916,7 +1916,7 @@ static int kvaser_usb_hydra_flush_queue(struct kvaser_usb_net_priv *priv)
 {
 	int err;
 
-	init_completion(&priv->flush_comp);
+	reinit_completion(&priv->flush_comp);
 
 	err = kvaser_usb_hydra_send_simple_cmd(priv->dev, CMD_FLUSH_QUEUE,
 					       priv->channel);
Signature · 1 line
2.38.0
#3 lore raw

PATCH v5 3/4 can: kvaser_usb_leaf: Fix TX queue out of sync after restart

In reply to Jimmy Assarsson

From: Anssi Hannula <[email protected]>

The TX queue seems to be implicitly flushed by the hardware during bus-off or bus-off recovery, but the driver does not reset the TX bookkeeping.

Despite not resetting TX bookkeeping the driver still re-enables TX queue unconditionally, leading to "cannot find free context" / NETDEV_TX_BUSY errors if the TX queue was full at bus-off time.

Fix that by resetting TX bookkeeping on CAN restart.

Tested with 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778.

Fixes 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")

---
Changes in v5:
- Split series and rebased

Changes in v4:
- No changes

Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b

Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
- Removed explicit queue flush.

 drivers/net/can/usb/kvaser_usb/kvaser_usb.h      | 2 ++
 drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 +-
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb.h +2 -0
index 841da29cef93..f6c0938027ec 100644
@@ -178,6 +178,8 @@ struct kvaser_usb_dev_cfg {
 extern const struct kvaser_usb_dev_ops kvaser_usb_hydra_dev_ops;
 extern const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops;
 
+void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv);
+
 int kvaser_usb_recv_cmd(const struct kvaser_usb *dev, void *cmd, int len,
 			int *actual_len);
 
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +1 -1
index c2bce6773adc..e91648ed7386 100644
@@ -477,7 +477,7 @@ static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
 /* This method might sleep. Do not call it in the atomic context
  * of URB completions.
  */
-static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
+void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
 {
 	usb_kill_anchored_urbs(&priv->tx_submitted);
 	kvaser_usb_reset_tx_urb_contexts(priv);
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +2 -0
index 8e11cda85624..59c220ef3049 100644
@@ -1426,6 +1426,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
 
 	switch (mode) {
 	case CAN_MODE_START:
+		kvaser_usb_unlink_tx_urbs(priv);
+
 		err = kvaser_usb_leaf_simple_cmd_async(priv, CMD_START_CHIP);
 		if (err)
 			return err;
Signature · 1 line
2.38.0
#4 lore raw

PATCH v5 4/4 can: kvaser_usb_leaf: Fix CAN state after restart

In reply to Jimmy Assarsson

From: Anssi Hannula <[email protected]>

can_restart() expects CMD_START_CHIP to set the error state to ERROR_ACTIVE as it calls netif_carrier_on() immediately afterwards.

Otherwise the user may immediately trigger restart again and hit a BUG_ON() in can_restart().

Fix kvaser_usb_leaf set_mode(CMD_START_CHIP) to set the expected state.

Fixes 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")

---
Changes in v5:
- Split series and rebased

Changes in v4:
- No changes

Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b

Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")

 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +2 -0
index 59c220ef3049..50f2ac8319ff 100644
@@ -1431,6 +1431,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
 		err = kvaser_usb_leaf_simple_cmd_async(priv, CMD_START_CHIP);
 		if (err)
 			return err;
+
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
 		break;
 	default:
 		return -EOPNOTSUPP;
Signature · 1 line
2.38.0
#5 lore raw
In reply to Jimmy Assarsson
Quoted 4 lines · Jimmy Assarsson

Changes in v5:
- Split series [1], keept only critical bug fixes that should go into stable, since v4 got rejected [2].
Non-critical fixes are posted in a separate series.

Looks much better now. Added to linux-can/main.

Thanks,
Marc

Signature · 4 lines
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |