can: kvaser_usb: Various fixes
Subsystems (2)
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
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.
---
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
@@ -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
PATCH v5 2/4 can: kvaser_usb: Fix use of uninitialized completion
In reply to Jimmy AssarssonFrom: 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.
---
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
@@ -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
@@ -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
PATCH v5 3/4 can: kvaser_usb_leaf: Fix TX queue out of sync after restart
In reply to Jimmy AssarssonFrom: 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.
---
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
@@ -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
@@ -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
@@ -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
PATCH v5 4/4 can: kvaser_usb_leaf: Fix CAN state after restart
In reply to Jimmy AssarssonFrom: 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.
---
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
@@ -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
Marc Kleine-Budde
maintainerQuoted 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 |