Re: [PATCH v4 07/11] HID: spi_hid: add ACPI support for SPI over HID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi_hid_acpi_deassert_reset() silently fails to execute the ACPI _RST method, breaking device initialization.
- [High] Use-After-Free due to flawed cleanup ordering: the reset workqueue is cancelled before the HID device is destroyed.
- [High] The shared buffer `shid->response` is read without holding `output_lock`, leading to data corruption if concurrent HID requests occur.
--

commit 8f475e9d4e137ed8f5267ef4406acd64e1baca61
Author: Angela Czubak <acz@semihalf.com>
Subject: HID: spi_hid: add ACPI support for SPI over HID

This commit detects and adds ACPI support for SPI HID devices.

> diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
> new file mode 100644
> index 0000000000000..298e3ba44d8a0
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-acpi.c
> @@ -0,0 +1,253 @@
[ ... ]
> +static int spi_hid_acpi_deassert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_acpi_config *conf = container_of(ops,
> +							struct spi_hid_acpi_config,
> +							ops);
> +
> +	return device_reset(&conf->adev->dev);

[Severity: High]
Does device_reset() work correctly here?

Since conf->adev->dev is the internal ACPI device structure without its fwnode
set, ACPI_HANDLE() will return null, causing __device_reset() to skip the ACPI
reset.

Also, looking at spi_hid_dev_init(), the return value of deassert_reset()
is ignored:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_init() {
    ...
	shid->ops->deassert_reset(shid->ops);
    ...
}

Should it evaluate _RST directly or use &spi->dev instead, and should the
return value be checked to prevent hardware reset from silently failing?

> +}

[Severity: High]
Can the cleanup ordering lead to a use-after-free regression during removal?

In spi_hid_core_remove(), cancel_work_sync() is called before
spi_hid_stop_hid():

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_core_remove() {
    ...
	cancel_work_sync(&shid->reset_work);

	spi_hid_stop_hid(shid);
    ...
}

Since spi_hid_stop_hid() calls hid_destroy_device(), the HID device is still
accessible to userspace between the work cancellation and device destruction.

If a userspace process concurrently issues an ioctl during this window,
spi_hid_sync_request() will timeout, and the error handler in
spi_hid_get_request() could incorrectly schedule reset_work. Once removal
finishes and devm frees the shid structure, could the mistakenly scheduled
work execute and dereference the freed shid pointer?

Should spi_hid_stop_hid() be called before cancel_work_sync() to prevent this?

[Severity: High]
Can the shared buffer shid->response be read while corrupted if concurrent
HID requests occur?

In spi_hid_sync_request(), output_lock is acquired via scoped guard and is
implicitly released upon function return:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_sync_request() {
    ...
	guard(mutex)(&shid->output_lock);
    ...
}

Functions like spi_hid_ll_raw_request() call into spi_hid_sync_request()
and then read from shid->response after the lock has been released:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_ll_raw_request() {
    ...
	ret = spi_hid_sync_request(shid, ...);
    ...
	ret = min_t(size_t, len,
		    (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
	buf[0] = shid->response->body[3];
	memcpy(&buf[1], &shid->response->content, ret);
    ...
}

If multiple threads concurrently issue ioctls, could the second thread acquire
output_lock and overwrite shid->response with its own data before the first
thread has finished reading it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=7