Re: [PATCH 03/13] HID: ft260: add GPIO support on top of UART

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized GPIO shadow state (`dev->gpio`) corrupts the hardware configuration of unrelated pins during updates.
- [High] Unvalidated `cfg->uart_mode` read from the device is used as an array index, leading to an out-of-bounds read.
- [Medium] Setting a GPIO direction to output introduces a hardware glitch by splitting the operation into two non-atomic USB requests.
- [Medium] Setting the UART mode to OFF unintentionally disables all non-UART GPIOs.
--

--- Patch [3]: [PATCH 03/13] HID: ft260: add GPIO support on top of UART ---
commit d5e8f3410c2633ba90471af57e3193f95d3193d4
Author: Michael Zaidman <[email protected]>
    
HID: ft260: add GPIO support on top of UART
    
Add gpiochip support for the FT260 multifunctional pins, including
GPIO2/GPIOA/GPIOG function selection via sysfs, and enable the
available GPIOs according to the active UART mode.
    
Link: https://lore.kernel.org/lkml/[email protected]/

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index b6c408267a685..cc3b8f5791dea 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -906,6 +1023,322 @@ static const struct i2c_algorithm ft260_i2c_algo = {
>  	.functionality = ft260_functionality,
>  };
>  
> +static void ft260_gpio_en_set(struct ft260_device *dev, u16 bitmap)
> +{
> +	dev->gpio_en |= bitmap & FT260_GPIO_MASK;
> +}
> +
> +static void ft260_gpio_en_clr(struct ft260_device *dev, u16 bitmap)
> +{
> +	dev->gpio_en &= ~bitmap & FT260_GPIO_MASK;
> +}
> +
> +static void ft260_gpio_en_update(struct hid_device *hdev, u8 req, u8 value)
> +{
> +	u16 bitmap;
> +	struct ft260_device *dev = hid_get_drvdata(hdev);
> +
> +	switch (req) {
> +
> +	case FT260_SET_I2C_MODE:
> +		bitmap = FT260_GPIO_I2C_DEFAULT;
> +		break;
> +	case FT260_SET_UART_MODE:
> +		switch (value) {
> +		case FT260_UART_CFG_FLOW_CTRL_OFF:
> +			bitmap = (u16)FT260_GPIO_MASK;
> +			break;
[ ... ]
> +		}
> +		ft260_gpio_en_clr(dev, bitmap);
> +		bitmap = dev->gpio_uart_mode[value];
> +		ft260_gpio_en_set(dev, bitmap);
> +		goto exit;

[Severity: Medium]
Does setting the UART flow control to OFF unintentionally disable all non-UART
GPIOs? 

When value is FT260_UART_CFG_FLOW_CTRL_OFF (0), bitmap is set to
FT260_GPIO_MASK, which represents all 14 GPIO bits. Calling
ft260_gpio_en_clr() inadvertently clears all of these bits from dev->gpio_en.
Subsequently, only the FT260_GPIO_UART_MODE_0_SET bits are restored, which
permanently disables I2C and other multi-function GPIOs until they are
manually re-enabled.

[ ... ]
> +static int ft260_gpio_set(struct gpio_chip *gc, u32 offset, int value)
> +{
> +	int ret = 0;
> +	struct ft260_gpio_write_request_report rep;
> +	struct ft260_device *dev = gpiochip_get_data(gc);
> +	struct hid_device *hdev = dev->hdev;
> +
> +	if (offset >= FT260_GPIO_TOTAL) {
> +		hid_err(hdev, "%s: invalid offset %d\n", __func__, offset);
> +		return -EINVAL;
> +	}
> +
> +	ft260_dbg("offset %d val %d\n", offset, value);
> +
> +	mutex_lock(&dev->lock);
> +
> +	if (!(dev->gpio_en & (1 << offset))) {
> +		hid_err(hdev, "%s: wrong pin function %d\n", __func__, offset);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	rep.report = FT260_GPIO;
> +	rep.gpio = dev->gpio;
> +
> +	if (offset < FT260_GPIO_MAX) {
[ ... ]
> +	ret = ft260_hid_feature_report_set(hdev, (u8 *)&rep, sizeof(rep));

[Severity: High]
Does this corrupt the hardware configuration of unrelated pins?

The shadow state dev->gpio is zero-initialized via kzalloc during device
allocation and is never synced with the actual hardware state during
ft260_gpio_init(). Because the entire dev->gpio struct is written to the
device here, won't all unmodified pins be unintentionally configured to input
mode with value 0?

> +	if (unlikely(ret < 0)) {
> +		hid_err(hdev, "%s: cannot set GPIO: %d\n", __func__, ret);
> +		goto exit;
> +	}
> +
> +	dev->gpio = rep.gpio;
> +	ret = 0;
> +exit:
> +	mutex_unlock(&dev->lock);
> +	return ret;
> +}
> +
> +static int ft260_gpio_direction_set(struct gpio_chip *gc, u32 offset,
> +				    int value, int direction)
> +{
> +	int ret;
> +	struct ft260_gpio_read_request_report buf;
> +	struct ft260_gpio_write_request_report *rep;
> +	struct ft260_device *dev = gpiochip_get_data(gc);
> +	struct hid_device *hdev = dev->hdev;
> +
[ ... ]
> +	ret = ft260_hid_feature_report_set(hdev, (u8 *)rep, sizeof(*rep));
> +	if (unlikely(ret < 0)) {
> +		hid_err(hdev, "%s: cannot set GPIO: %d\n", __func__, ret);
> +		goto exit;
> +	}
> +
> +	dev->gpio = rep->gpio;
> +	mutex_unlock(&dev->lock);
> +
> +	if (direction == FT260_GPIO_DIR_OUTPUT)
> +		ft260_gpio_set(gc, offset, value);

[Severity: Medium]
Could this introduce a hardware glitch when changing a pin's direction to
output? 

The code updates the direction bits to output without setting the corresponding
value bits, and writes this immediately to the device via
ft260_hid_feature_report_set(). Then it sends a second USB request via
ft260_gpio_set() to set the desired output value. During the window between
these two USB transfers, will the pin assert an unintended logic level?

> +
> +	return 0;
> +exit:
> +	mutex_unlock(&dev->lock);
> +	return ret;
> +}
[ ... ]
> +static int ft260_gpio_init(struct ft260_device *dev,
> +			   struct ft260_get_system_status_report *cfg)
> +{
> +	int ret;
> +	int label_sz;
> +	char *label;
> +	struct ft260_get_chip_version_report ver;
> +	struct hid_device *hdev = dev->hdev;
> +	char prefix[] = "ft260_";
> +	u8 mode = cfg->chip_mode;
> +
> +	hid_info(hdev, "initialize gpio chip\n");
> +
> +	dev->gpio_uart_mode[0] = (u16)FT260_GPIO_UART_MODE_0_SET;
> +	dev->gpio_uart_mode[1] = (u16)FT260_GPIO_UART_MODE_1_SET;
> +	dev->gpio_uart_mode[2] = (u16)FT260_GPIO_UART_MODE_2_SET;
> +	dev->gpio_uart_mode[3] = (u16)FT260_GPIO_UART_MODE_3_SET;
> +	dev->gpio_uart_mode[4] = (u16)FT260_GPIO_UART_MODE_4_SET;
> +
> +	if (mode == FT260_MODE_ALL)
> +		mode = FT260_MODE_BOTH;
> +
> +	if (mode & FT260_MODE_UART)
> +		dev->gpio_en |= dev->gpio_uart_mode[cfg->uart_mode];

[Severity: High]
Can this read out of bounds? 

cfg->uart_mode is read directly from the device via a USB feature report in
ft260_get_system_config(). If a malicious or malfunctioning USB device returns
a uart_mode >= 5, won't this leak kernel heap data into dev->gpio_en or crash
since dev->gpio_uart_mode only has 5 elements?

> +	else
> +		dev->gpio_en |= FT260_GPIO_UART_DEFAULT;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3