Messages in this thread |  | | Date | Sat, 28 May 2022 09:34:47 +0200 | From | Jimmy Assarsson <> | Subject | Re: [PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command |
| |
On 5/16/22 15:47, Anssi Hannula wrote: > 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") > Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Looks good to me. Tested-by: Jimmy Assarsson <extja@kvaser.com>
> --- > > A simpler option would be to just zero-pad the data into a > stack-allocated struct kvaser_cmd without knowledge of the needed > minimum size (so no tables needed), though that would mean incomplete > commands would get passed on silently. > > .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c > index c805b999c543..d9f40b9702a5 100644 > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c > @@ -320,6 +320,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: > * > @@ -387,6 +419,43 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz = { > .bittiming_const = &kvaser_usb_leaf_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->card_data.leaf.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, > @@ -492,6 +561,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; > } > > @@ -1113,6 +1185,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);
|  |