lkml.org 
[lkml]   [2022]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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);

    \
     
     \ /
      Last update: 2022-05-28 09:36    [W:4.081 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site