Re: [PATCH v2 2/2] HID: lenovo: suppress phantom radio control input on Yoga Slim 7x Gen 11

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting: `lenovo_remove()` incorrectly cancels asynchronous work before stopping the hardware, which can cause a Use-After-Free.
- [High] Preexisting: In `lenovo_probe_tpkbd()`, sysfs attributes are registered and exposed to userspace before the required private driver data is allocated.
--

commit 5bdafd669d3015385ef2a78a1df43a3a45427c70
Author: Oleg Keri <okerixx@gmail.com>

HID: lenovo: suppress phantom radio control input on Yoga Slim 7x Gen 11

This commit suppresses a phantom radio control input on the Yoga Slim 7x
Gen 11 by mapping the Wireless Radio Controls application collection to
nothing, avoiding an unnecessary KEY_RFKILL source.

> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 3976d7b53b14b..617bba6626592 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does lenovo_remove()
incorrectly cancel asynchronous work before stopping the hardware,
risking a use-after-free?

In lenovo_remove(), lenovo_remove_tp10ubkbd() is called before
hid_hw_stop():

drivers/hid/hid-lenovo.c:lenovo_remove() {
    ...
		lenovo_remove_tp10ubkbd(hdev);
		break;
	}

	hid_hw_stop(hdev);
    ...
}

This means hardware interrupts are still active when the work is cancelled:

drivers/hid/hid-lenovo.c:lenovo_remove_tp10ubkbd() {
    ...
	cancel_work_sync(&data->fn_lock_sync_work);
    ...
}

If an event like KEY_FN_ESC triggers lenovo_event_tp10ubkbd() in this window,
it will re-queue the work:

drivers/hid/hid-lenovo.c:lenovo_event_tp10ubkbd() {
    ...
		schedule_work(&data->fn_lock_sync_work);
    ...
}

Devres subsequently frees the data structure when remove() exits.
Will the re-queued work then run and access the freed data structure?


[Severity: High]
This is a pre-existing issue, but does lenovo_probe_tpkbd() expose sysfs
attributes before allocating required private driver data?

In lenovo_probe_tpkbd(), sysfs_create_group() exposes attributes like
sensitivity to userspace before driver data is allocated:

drivers/hid/hid-lenovo.c:lenovo_probe_tpkbd() {
    ...
	ret = sysfs_create_group(&hdev->dev.kobj, &lenovo_attr_group_tpkbd);
	if (ret)
		hid_warn(hdev, "Could not create sysfs group: %d\n", ret);

	data_pointer = devm_kzalloc(&hdev->dev,
				    sizeof(struct lenovo_drvdata),
				    GFP_KERNEL);
	if (data_pointer == NULL) {
    ...
}

If userspace reads a sysfs file immediately after it appears, the read
callback will blindly dereference a NULL pointer:

drivers/hid/hid-lenovo.c:attr_sensitivity_show_tpkbd() {
    ...
	struct hid_device *hdev = to_hid_device(dev);
	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);

	return sysfs_emit(buf, "%u\n", data_pointer->sensitivity);
}

Can this lead to a NULL pointer dereference if the files are accessed
during device probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910063051.4556-1-okerixx@gmail.com?part=2