Re: [PATCH 05/13] HID: ft260: i2c: silence sysfs store big-numbers

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The patch unconditionally overwrites the error code from `ft260_hid_feature_report_set()` with `count`, masking hardware write errors from userspace.

Pre-existing issues:
- [Critical] Double free on UART probe error path and Use-After-Free during device remove.
- [High] Native endianness is incorrectly leaked to the device when parsing multi-byte sysfs attributes.
- [High] Teardown order of the wakeup timer and workqueue during device removal allows a Use-After-Free via a re-scheduled work item.
- [High] Manually freeing `dev` with `kfree()` while a devres-managed GPIO chip is still registered creates a use-after-free window.
--

--- Patch [5]: [PATCH 05/13] HID: ft260: i2c: silence sysfs store big-numbers ---
commit ebf18ef9c09c87f02215b8dbbbe0f60a02ef6565
Author: Michael Zaidman <[email protected]>

    HID: ft260: i2c: silence sysfs store big-numbers
    
    FT260_ATTR_STORE returned the feature-report transfer length from
    ft260_hid_feature_report_set() instead of the sysfs write size. For
    values like 1000, userspace writes more bytes than the HID report
    length, so the VFS retries the remainder and reports "Invalid argument"
    even though the attribute was applied correctly:
    
      $ echo 1000 > .../clock
      bash: line 1: echo: write error: Invalid argument
      $ cat .../clock
      1000
    
    Return count after a successful parse so the full write is consumed.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 4435a39fce239..8db896f164cd0 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -1479,6 +1479,7 @@ static void ft260_attr_dummy_func(struct hid_device *hdev, u8 req, u16 value)
>  				hid_err(hdev, "%s: failed!\n", __func__);      \
>  			else						       \
>  				func(hdev, req, name);			       \
> +			ret = count;					       \
>  			mutex_unlock(&dev->lock);			       \
>  		} else {						       \
>  			ret = -EINVAL;					       \

[Severity: High]
Does this unconditionally overwrite the error code from
ft260_hid_feature_report_set()? 

Since the else branch lacks curly braces, it appears ret = count executes
even if ret < 0. Could this mask hardware write errors from userspace by
incorrectly telling the VFS that the write succeeded?

[Severity: High]
This isn't a bug introduced by this patch, but does FT260_WORD_ATTR_STORE()
leak native endianness to the device? 

drivers/hid/hid-ft260.c:
#define FT260_WORD_ATTR_STORE(name, reptype, req, func)		       \
		FT260_ATTR_STORE(name, reptype, FT260_SYSTEM_SETTINGS, req,    \
				 __le16, u16, kstrtou16, func)

It passes a __le16 type directly to kstrtou16(), which seems to write the value
in native byte order, assigning it to the little-endian report structure
without cpu_to_le16() conversion. Could this cause incorrect device
configuration on big-endian architectures?

[Severity: Critical]
This is a pre-existing issue, but does ft260_probe() have a double free on
the UART probe error path?

drivers/hid/hid-ft260.c:ft260_probe()
hid_fail:
	kfree(dev);
	return ret;

If ft260_uart_probe() fails after tty_port_init(), ft260_uart_port_remove()
calls tty_port_put(), destroying the port and freeing dev. The error path
then calls kfree(dev).

Similarly, in ft260_remove(), does this sequence cause a use-after-free?

drivers/hid/hid-ft260.c:ft260_remove()
		ft260_uart_port_remove(dev);
		/* dev is still needed, so we will free it in _destroy func */
		if (dev->chip_mode & FT260_MODE_UART ||

This dereferences dev->chip_mode immediately after ft260_uart_port_remove()
has dropped the final port reference and freed dev.

[Severity: High]
This is a pre-existing issue, but could the teardown order of the wakeup timer
and workqueue in ft260_remove() allow a use-after-free?

drivers/hid/hid-ft260.c:ft260_remove()
		cancel_work_sync(&dev->wakeup_work);
		tty_unregister_device(ft260_tty_driver,
					   dev->index);
		ft260_uart_port_remove(dev);

The workqueue is cancelled before the timer is deleted inside
ft260_uart_port_remove(). If the timer fires in this window, could it check
dev->reschedule_work and reschedule the work item, which would then execute
after dev is freed?

[Severity: High]
This is a pre-existing issue, but does manually freeing dev with kfree()
while a devres-managed GPIO chip is still registered create a use-after-free
window?

drivers/hid/hid-ft260.c:ft260_remove()
		i2c_del_adapter(&dev->adap);
		kfree(dev);
	}

And in the ft260_probe() error path:

drivers/hid/hid-ft260.c:ft260_probe()
hid_fail:
	kfree(dev);
	return ret;

Since the GPIO chip is managed by devm_gpiochip_add_data(), it remains
registered until the core processes devres actions after the function returns,
leaving a window where the GPIO chip can be accessed while its backing data
(dev) has been freed.

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