Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <20ae75a3-13a9-a7a2-6b0d-3df73703e001@kvaser.com>
Date:   Sat, 28 May 2022 09:34:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
From:   Jimmy Assarsson <extja@kvaser.com>
Subject: Re: [PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid
 command
To:     Anssi Hannula <anssi.hannula@bitwise.fi>
Cc:     linux-can@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-kernel@vger.kernel.org
References: <20220516134748.3724796-1-anssi.hannula@bitwise.fi>
 <20220516134748.3724796-2-anssi.hannula@bitwise.fi>
Content-Language: en-US
In-Reply-To: <20220516134748.3724796-2-anssi.hannula@bitwise.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);