Re: [PATCH] HID: i2c-hid: add quirk to send initial output reports for Acer SFA14-I31

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: i2c-hid: silent quirk failure due to dependency on synchronous driver binding
- [High] hid: i2c-hid: unprotected report_list traversal causes data race and UAF
--

commit 8bcb0b187653aa75d332fb5ab3f4c9d75928faf8
Author: DMS <dms3studio@gmail.com>

HID: i2c-hid: add quirk to send initial output reports for Acer SFA14-I31

The internal keyboard of the Acer Swift Air 14 (SFA14-I31, ACPI
1025174B, HID 049f:5343) does not send any input reports after probe.
It stays silent, and its GPIO interrupt never fires, until the host has
written an output report to it at least once.

Add I2C_HID_QUIRK_INIT_OUTPUT_REPORTS, which sends every output report
once with its initial field values right after the HID device has been
added, and enable it for this keyboard.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index a8a9854854f52..f546710925e64 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c

[ ... ]

> @@ -1079,6 +1086,32 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
>  	return 0;
>  }
>  
> +/*
> + * The input core only writes LED state when it changes (and on resume), so
> + * a device that waits for an output report never gets one at boot. Send
> + * every output report once with its initial field values.
> + */
> +static void i2c_hid_send_initial_output_reports(struct hid_device *hid)
> +{
> +	struct hid_report_enum *report_enum = &hid->report_enum[HID_OUTPUT_REPORT];
> +	struct hid_report *report;
> +	u8 *buf;
> +	int ret;
> +
> +	list_for_each_entry(report, &report_enum->report_list, list) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this unsynchronized iteration risk a data race or use-after-free?

Because hid_add_device() has already exposed the device to the driver core
and userspace, the high-level driver could be binding asynchronously and
mutating the list via hid_register_report(), or unbinding and freeing the
reports concurrently via hid_close_report().

[ ... ]

> @@ -1099,6 +1132,9 @@ static int i2c_hid_core_register_hid(struct i2c_hid *ihid)
>  	if (ihid->quirks & I2C_HID_QUIRK_RE_POWER_ON)
>  		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
>  
> +	if (ihid->quirks & I2C_HID_QUIRK_INIT_OUTPUT_REPORTS)
> +		i2c_hid_send_initial_output_reports(hid);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Will this reliably send the initial reports?

Because this executes immediately after hid_add_device() registers the
device, reports are not parsed and added to report_list yet. The list is
only populated when the high-level driver (e.g. hid-generic) probes and
calls hid_parse().

If the high-level driver binds asynchronously or is loaded as a module
after i2c-hid, i2c_hid_send_initial_output_reports() will iterate over an
empty list and silently do nothing, leaving the keyboard unresponsive.

> +
>  	return ret;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-swift-sfa14-kbd-v1-1-ad312bca1076@gmail.com?part=1