| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20ae75a3-13a9-a7a2-6b0d-3df73703e001@kvaser.com>
Date: Sat, 28 May 2022 09:34:47 +0200
From: Jimmy Assarsson <extja@...ser.com>
To: Anssi Hannula <anssi.hannula@...wise.fi>
Cc: linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
linux-kernel@...r.kernel.org
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@...wise.fi>
Looks good to me.
Tested-by: Jimmy Assarsson <extja@...ser.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);
Powered by blists - more mailing lists