* Re: [PATCH v2] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware
From: srinivas pandruvada @ 2026-07-20 20:59 UTC (permalink / raw)
To: Wahid7852, linux-input; +Cc: jikos, Lixu Zhang
In-Reply-To: <20260720175656.559351-1-wahidzk0091@gmail.com>
On Mon, 2026-07-20 at 23:26 +0530, Wahid7852 wrote:
> From: Wahid Khan <wahidzk0091@gmail.com>
>
> Some ISH firmware versions (observed on Tiger Lake LP, 8086:a0fc,
> GUID {33AECD58-B679-4E54-9BD9-A04D34F0C226}) periodically re-send an
> unsolicited HOSTIF_DM_ENUM_DEVICES response roughly every 79 seconds.
>
> The current guard collapses two distinct cases into one condition:
>
> if (!(response_flag) || init_done) -> bad packet + ish_hw_reset
>
> This incorrectly treats a valid post-init firmware announcement as a
> corrupted packet, triggering an ISH soft-reset and a full ISHTP
> re-initialisation cycle on each occurrence (~1100 times per day on
> affected hardware).
>
> Split the check: reject messages with no response flag as before, but
> simply discard valid ENUM_DEVICES messages that arrive after init is
> complete.
>
> Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Lixu, please add your tested by. I think you already tested this.
Thanks,
Srinivas
> ---
> v2: added proper sign off
>
> drivers/hid/intel-ish-hid/ishtp-hid-client.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6d64008f2..ec5e8afe7 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -113,8 +113,7 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
>
> switch (recv_msg->hdr.command & CMD_MASK) {
> case HOSTIF_DM_ENUM_DEVICES:
> - if ((!(recv_msg->hdr.command & ~CMD_MASK) ||
> - client_data->init_done)) {
> + if (!(recv_msg->hdr.command & ~CMD_MASK)) {
> ++client_data->bad_recv_cnt;
> report_bad_packet(hid_ishtp_cl,
> recv_msg,
> cur_pos,
> @@ -122,6 +121,8 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> ish_hw_reset(ishtp_get_ishtp_device(
> hid_ishtp_cl));
> break;
> }
> + if (client_data->init_done)
> + break;
> client_data->hid_dev_count = (unsigned
> int)*payload;
> if (!client_data->hid_devices)
> client_data->hid_devices =
> devm_kcalloc(
> --
> 2.54.0
^ permalink raw reply
* Re: [PATCH 02/10] iio: gyro: hid-sensor-gyro-3d: use local struct device
From: Andy Shevchenko @ 2026-07-20 19:35 UTC (permalink / raw)
To: Sanjay Chitroda
Cc: Jiri Kosina, Jonathan Cameron, Srinivas Pandruvada, David Lechner,
Nuno Sá, Andy Shevchenko, linux-input, linux-iio,
linux-kernel
In-Reply-To: <1E009860-0A0F-4F8D-8017-CE5739F22144@gmail.com>
On Mon, Jul 20, 2026 at 10:20:28PM +0530, Sanjay Chitroda wrote:
> On 20 July 2026 5:24:33 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >On Mon, Jul 20, 2026 at 03:10:14PM +0530, Sanjay Chitroda via B4 Relay wrote:
...
> >> - dev_err(&pdev->dev, "failed to setup common attributes\n");
> >> + dev_err(dev, "failed to setup common attributes\n");
> >> return ret;
> >
> >Nope, First one (to eliminate a lot of churn) should be to convert this and
> >similar to use
> >
> > return dev_err_probe(...);
>
> Thank you for your input.
>
> I'm planning to have a incremental series: first introduce a local "dev"
> pointer, then convert the resource management to "devm_*", and finally
> replace the remaining error reporting with "dev_err_probe()".
>
> This keeps each series focused and avoids introducing intermediate patterns such as:
>
> ret = dev_err_probe(dev, ret, ...);
> goto error;
>
> Once the cleanup paths are removed, those sites naturally become:
>
> return dev_err_probe(dev, ret, ...);
>
> Does this approach sound reasonable?
No. First you should drop the dead code for sure (see above).
Second, try to convert to dev_err_probe() as many drivers as you can without an
intermediate change. If the driver requires an intermediate thing like the above
it means it requires bigger refactoring. So, you should not have intermediate
churn. With your current approach there are drivers that will have it, and for
them you need to have different strategy (see my proposal in the above lines).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware
From: sashiko-bot @ 2026-07-20 18:18 UTC (permalink / raw)
To: Wahid7852; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260720175656.559351-1-wahidzk0091@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Integer underflow and pointer arithmetic bugs in `HOSTIF_PUBLISH_INPUT_REPORT_LIST` allow massive out-of-bounds memory reads.
- [Critical] Missing bounds check on `hid_dev_count` allows an out-of-bounds write on fixed-size arrays.
- [High] Missing state validation for duplicate initialization responses causes a heap buffer overflow due to skipped reallocations.
- [High] Inadequate bounds checking in `HOSTIF_DM_ENUM_DEVICES` allows out-of-bounds memory reads from the receive buffer.
- [High] The driver fails to cancel `client_data->work` and `client_data->resume_work` during device removal, leading to a potential use-after-free.
--
--- Patch [1]: [PATCH v2] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware ---
commit f5198129202a132955d59473ed96cfb8c860906b
Author: Wahid Khan <wahidzk0091@gmail.com>
HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware
This commit modifies the guard in process_recv() for the
HOSTIF_DM_ENUM_DEVICES command. It splits the check to reject messages
with no response flag, while silently discarding valid ENUM_DEVICES
messages that arrive after initialization is complete.
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6d64008f2ce05..ec5e8afe77bd4 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -113,8 +113,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>
> switch (recv_msg->hdr.command & CMD_MASK) {
> case HOSTIF_DM_ENUM_DEVICES:
> - if ((!(recv_msg->hdr.command & ~CMD_MASK) ||
> - client_data->init_done)) {
> + if (!(recv_msg->hdr.command & ~CMD_MASK)) {
> ++client_data->bad_recv_cnt;
> report_bad_packet(hid_ishtp_cl, recv_msg,
> cur_pos,
> @@ -122,6 +121,8 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
> ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
> break;
> }
> + if (client_data->init_done)
> + break;
> client_data->hid_dev_count = (unsigned int)*payload;
> if (!client_data->hid_devices)
> client_data->hid_devices = devm_kcalloc(
[Severity: High]
This is a pre-existing issue, but could we experience a heap buffer overflow
in process_recv() if we receive a duplicate HOSTIF_DM_ENUM_DEVICES response
before initialization completes?
The payload length or count is updated unconditionally, but reallocation
is skipped if the buffer already exists:
client_data->hid_dev_count = (unsigned int)*payload;
if (!client_data->hid_devices)
client_data->hid_devices = devm_kcalloc(...)
Would a subsequent response with a larger device count cause the memcpy loop
to write beyond the initially allocated bounds of client_data->hid_devices?
[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check on the
unvalidated device count provided by the firmware in process_recv()?
The firmware provides an unvalidated hid_dev_count (up to 255) during
HOSTIF_DM_ENUM_DEVICES, which the driver stores and uses as num_hid_devices.
When processing responses for GET_HID_DESCRIPTOR or GET_REPORT_DESCRIPTOR,
the driver uses curr_hid_dev to allocate and write into statically sized
arrays like client_data->hid_descr:
client_data->hid_descr[curr_hid_dev] = devm_kmalloc(cl_data_to_dev(client_data), payload_len, GFP_KERNEL);
Since these arrays are statically sized to MAX_HID_DEVICES (32), could a
device count larger than 32 result in a critical out-of-bounds write on the
client_data heap structure?
[Severity: High]
This is a pre-existing issue, but does inadequate bounds checking during
the parsing of HOSTIF_DM_ENUM_DEVICES in process_recv() allow out-of-bounds
memory reads from the receive buffer?
When processing ENUM_DEVICES, the loop validates the bounds of incoming
device_info structures using data_len instead of payload_len:
if (1 + sizeof(struct device_info) * i >= data_len)
break;
Additionally, if the offset exceeds payload_len, the code logs an error
but fails to break out of the loop:
if (1 + sizeof(struct device_info) * i >= payload_len) {
dev_err(cl_data_to_dev(client_data), ...);
}
Can this cause the driver to read beyond the end of the current ISHTP packet,
and potentially beyond the recv_buf allocation entirely when computing the
pointer for memcpy?
dev_info = (struct device_info *)(payload + 1 + sizeof(struct device_info) * i);
[Severity: Critical]
This is a pre-existing issue, but are there integer underflow and pointer
arithmetic bugs in the HOSTIF_PUBLISH_INPUT_REPORT_LIST case of process_recv()
that could allow massive out-of-bounds memory reads?
If an incoming report->size is less than sizeof(struct hostif_msg_hdr),
can payload_len underflow to a massive value?
payload_len = report_len - sizeof(struct hostif_msg_hdr);
Furthermore, even without underflow, does advancing the pointer using C typed
pointer arithmetic implicitly multiply the increment by sizeof(struct report)?
report += sizeof(*report) + payload_len;
Would this advance the pointer wildly out-of-bounds, feeding garbage lengths
and pointers to hid_input_report(), potentially leading to kernel panics or
data leakage?
[Severity: High]
This is a pre-existing issue, but does the driver fail to cancel pending work
during device removal, leading to a potential use-after-free?
The driver queues hardware reset and resume handlers on an ISHTP-wide shared
workqueue. However, hid_ishtp_cl_remove() exits without calling
cancel_work_sync() for client_data->work and client_data->resume_work:
drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_remove() {
...
ishtp_hid_remove(client_data);
hid_ishtp_cl_deinit(hid_ishtp_cl);
...
}
When devres frees client_data, might any concurrently running or pending work
on the shared workqueue access the freed structure?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720175656.559351-1-wahidzk0091@gmail.com?part=1
^ permalink raw reply
* [PATCH v2] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware
From: Wahid7852 @ 2026-07-20 17:56 UTC (permalink / raw)
To: linux-input; +Cc: srinivas.pandruvada, jikos, Lixu Zhang, Wahid Khan
In-Reply-To: <20260627224753.35075-1-wahidzk0091@gmail.com>
From: Wahid Khan <wahidzk0091@gmail.com>
Some ISH firmware versions (observed on Tiger Lake LP, 8086:a0fc,
GUID {33AECD58-B679-4E54-9BD9-A04D34F0C226}) periodically re-send an
unsolicited HOSTIF_DM_ENUM_DEVICES response roughly every 79 seconds.
The current guard collapses two distinct cases into one condition:
if (!(response_flag) || init_done) -> bad packet + ish_hw_reset
This incorrectly treats a valid post-init firmware announcement as a
corrupted packet, triggering an ISH soft-reset and a full ISHTP
re-initialisation cycle on each occurrence (~1100 times per day on
affected hardware).
Split the check: reject messages with no response flag as before, but
simply discard valid ENUM_DEVICES messages that arrive after init is
complete.
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
v2: added proper sign off
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 6d64008f2..ec5e8afe7 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -113,8 +113,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
switch (recv_msg->hdr.command & CMD_MASK) {
case HOSTIF_DM_ENUM_DEVICES:
- if ((!(recv_msg->hdr.command & ~CMD_MASK) ||
- client_data->init_done)) {
+ if (!(recv_msg->hdr.command & ~CMD_MASK)) {
++client_data->bad_recv_cnt;
report_bad_packet(hid_ishtp_cl, recv_msg,
cur_pos,
@@ -122,6 +121,8 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
+ if (client_data->init_done)
+ break;
client_data->hid_dev_count = (unsigned int)*payload;
if (!client_data->hid_devices)
client_data->hid_devices = devm_kcalloc(
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] Input: st-keyscan: Improve probe error handling
From: Dmitry Torokhov @ 2026-07-20 17:08 UTC (permalink / raw)
To: phucduc.bui; +Cc: linux-input, linux-kernel
In-Reply-To: <20260720090851.230244-2-phucduc.bui@gmail.com>
On Mon, Jul 20, 2026 at 04:08:50PM +0700, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Use dev_err_probe() for devm_clk_get() so deferred probe errors are
> handled correctly.
Same here: in what way the does existing code handle deferred probe
incorrectly?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] Input: snvs_pwrkey: Improve probe error handling
From: Dmitry Torokhov @ 2026-07-20 17:07 UTC (permalink / raw)
To: phucduc.bui; +Cc: linux-input, linux-kernel
In-Reply-To: <20260720090851.230244-1-phucduc.bui@gmail.com>
On Mon, Jul 20, 2026 at 04:08:49PM +0700, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Use dev_err_probe() for devm_clk_get_optional_enabled() to handle
> deferred probe correctly.
In what way the does existing code handle deferred probe incorrectly?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 02/10] iio: gyro: hid-sensor-gyro-3d: use local struct device
From: Sanjay Chitroda @ 2026-07-20 16:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Kosina, Jonathan Cameron, Srinivas Pandruvada, David Lechner,
Nuno Sá, Andy Shevchenko, linux-input, linux-iio,
linux-kernel
In-Reply-To: <al4MeTgAWWkJsb1T@ashevche-desk.local>
On 20 July 2026 5:24:33 pm IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Mon, Jul 20, 2026 at 03:10:14PM +0530, Sanjay Chitroda via B4 Relay wrote:
>
>> Introduce a local struct device pointer derived from &pdev->dev.
>> This avoids repeated &pdev->dev usage and improves readability.
>
>...
>
>> if (ret) {
>> - dev_err(&pdev->dev, "failed to setup common attributes\n");
>> + dev_err(dev, "failed to setup common attributes\n");
>> return ret;
>
>Nope, First one (to eliminate a lot of churn) should be to convert this and
>similar to use
>
> return dev_err_probe(...);
>
Hi Andy,
Thank you for your input.
I'm planning to have a incremental series: first introduce a local "dev" pointer, then convert the resource management to "devm_*", and finally replace the remaining error reporting with "dev_err_probe()".
This keeps each series focused and avoids introducing intermediate patterns such as:
ret = dev_err_probe(dev, ret, ...);
goto error;
Once the cleanup paths are removed, those sites naturally become:
return dev_err_probe(dev, ret, ...);
Does this approach sound reasonable?
>> }
>>
>> - indio_dev->channels = devm_kmemdup(&pdev->dev, gyro_3d_channels,
>> + indio_dev->channels = devm_kmemdup(dev, gyro_3d_channels,
>> sizeof(gyro_3d_channels), GFP_KERNEL);
>> if (!indio_dev->channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> + dev_err(dev, "failed to duplicate channels\n");
>
>And this kind of messages are dead code and must be just removed completely.
>
>> return -ENOMEM;
>> }
>
>...
>
>Please, revisit the whole series.
Yep, understood.
I'll revisit series, drop dead code and send v2.
Thanks, Sanjay
>
^ permalink raw reply
* Re: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain
From: sashiko-bot @ 2026-07-20 16:46 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260720162219.32324-2-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] The patch attempts to prevent indefinite lockups during driver unbind or PM transitions by adding a timeout to the drain waits. However, upon timeout, it explicitly falls back to waiting unconditionally on the exact same condition, neutralizing the protection and still causing an indefinite lockup.
- [Low] Unnecessary removal of scope-based cleanup `guard()`, reverting to manual spinlock management.
Pre-existing issues:
- [Critical] Missing cancel_work_sync() in applespi_remove() causes Use-After-Free of applespi_data when the workqueue executes.
--
--- Patch [1]: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain ---
commit 08fd2aeb0171b9531113cd115a28c72f63462dc0
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - use unified wait queue with timeouts for drain
Consolidate wait queues by renaming drain_complete to wait_queue and use
wait_event_lock_irq_timeout() with a 3-second timeout during read and write
drains to prevent indefinite lockups during driver unbind or PM transitions
if the hardware becomes unresponsive.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..c501bf7e517e4 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -1793,21 +1793,49 @@ static int applespi_probe(struct spi_device *spi)
>
> static void applespi_drain_writes(struct applespi_data *applespi)
> {
> - guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> + unsigned long flags;
> + long ret;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
[Severity: Low]
This isn't a bug, but is it necessary to revert from guard() to manual
spin_lock_irqsave() here?
The input subsystem guidelines prefer using modern cleanup primitives like
guard() in new or refactored code. wait_event_lock_irq_timeout() should
work correctly with the hidden flags variable established by guard().
>
> applespi->drain = true;
> - wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
> - applespi->cmd_msg_lock);
> + ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> + !applespi->write_active,
> + applespi->cmd_msg_lock,
> + msecs_to_jiffies(3000));
> + if (!ret && applespi->write_active) {
> + dev_warn(&applespi->spi->dev,
> + "Timed out waiting for write drain, waiting unconditionally\n");
> + wait_event_lock_irq(applespi->wait_queue,
> + !applespi->write_active,
> + applespi->cmd_msg_lock);
[Severity: Medium]
Does this fallback logic defeat the purpose of the timeout?
The commit message states the timeout is meant to prevent indefinite lockups,
but if a timeout occurs here, wait_event_lock_irq() waits unconditionally
on the exact same condition.
If the hardware is genuinely unresponsive, won't this just delay the indefinite
lockup by 3 seconds rather than preventing it?
> + }
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> }
>
> static void applespi_drain_reads(struct applespi_data *applespi)
> {
> - guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> + unsigned long flags;
> + long ret;
>
> - wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
> - applespi->cmd_msg_lock);
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> + !applespi->read_active,
> + applespi->cmd_msg_lock,
> + msecs_to_jiffies(3000));
> + if (!ret && applespi->read_active) {
> + dev_warn(&applespi->spi->dev,
> + "Timed out waiting for read drain, waiting unconditionally\n");
> + wait_event_lock_irq(applespi->wait_queue,
> + !applespi->read_active,
> + applespi->cmd_msg_lock);
[Severity: Medium]
Does this read drain fallback have the same indefinite lockup issue as the
write drain when the hardware fails?
> + }
>
> applespi->suspended = true;
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> }
>
> static void applespi_remove(struct spi_device *spi)
[Severity: Critical]
This is a pre-existing issue, but does applespi_remove() miss a call to
cancel_work_sync(&applespi->work) before returning?
If the driver unbinds while a touchpad info SPI transaction is in flight,
applespi_drain_reads() will block until the read finishes.
When the SPI read completes, its handler schedules applespi->work, clears
read_active, and wakes up the drain.
If applespi_remove() finishes without cancelling the work, devres will
free applespi_data, which could lead to a use-after-free when the scheduled
applespi_worker() executes.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720162219.32324-1-fourdollars@debian.org?part=1
^ permalink raw reply
* Re: [PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing
From: sashiko-bot @ 2026-07-20 16:38 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260720162219.32324-5-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent data race on the global `touchpad_dimensions` array due to asynchronous probing.
--
--- Patch [4]: [PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing ---
commit b9f1c9a61a0fcf093f49319a208df622cfeba00e
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - prefer asynchronous driver probing
This commit enables asynchronous probing for the applespi driver to avoid
blocking the main kernel thread. It also cleans up product ID retrieval
in applespi_tp_dim_open().
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -2044,6 +2045,7 @@ static struct spi_driver applespi_driver = {
> .name = "applespi",
> .acpi_match_table = applespi_acpi_match,
> .pm = pm_sleep_ptr(&applespi_pm_ops),
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
[Severity: Medium]
Could enabling asynchronous probing here cause a data race on the global
touchpad_dimensions array?
Looking at applespi_register_touchpad_device(), it reads from and modifies
the global touchpad_dimensions module parameter without locking:
drivers/input/keyboard/applespi.c:applespi_register_touchpad_device()
sts = sscanf(touchpad_dimensions, "%dx%d+%u+%u", &x, &y, &w, &h);
...
touchpad_dimensions[0] = '\0';
...
snprintf(touchpad_dimensions, sizeof(touchpad_dimensions), ...
If a system has multiple instances of the APP000D ACPI device, the driver
core could probe them concurrently, potentially corrupting this string or
causing incorrect dimensions to be read and parsed.
> },
> .probe = applespi_probe,
> .remove = applespi_remove,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720162219.32324-1-fourdollars@debian.org?part=4
^ permalink raw reply
* Re: [PATCH v3 5/5] Input: applespi - fix use-after-free in applespi_remove()
From: sashiko-bot @ 2026-07-20 16:37 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260720162219.32324-6-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Improper mixing of `spin_lock_irqsave` with `wait_event_lock_irq` variants.
Pre-existing issues:
- [High] The driver fails to clear the `cancel_spi` flag during resume, rendering the keyboard and touchpad permanently dead after the first suspend cycle.
--
--- Patch [5]: [PATCH v3 5/5] Input: applespi - fix use-after-free in applespi_remove() ---
commit dde1844e9ce22b6dd80fd613693670eb010fc66b
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - fix use-after-free in applespi_remove()
Replace the separate read and write drain helpers with a single barrier using
cancel_spi and wait_event_lock_irq_timeout(). Wait for outstanding
asynchronous SPI operations to complete before tearing down the driver
context.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index a21e89f303879..6027808421247 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1939,14 +1939,29 @@ static void applespi_drain_reads(struct applespi_data *applespi)
> static void applespi_remove(struct spi_device *spi)
> {
> struct applespi_data *applespi = spi_get_drvdata(spi);
> + unsigned long flags;
> + long ret;
>
> - applespi_drain_writes(applespi);
> -
> + /* Disable GPE and remove handler first to prevent interrupt storm */
> acpi_disable_gpe(NULL, applespi->gpe);
> acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
> device_wakeup_disable(&spi->dev);
>
> - applespi_drain_reads(applespi);
> + /* Prevent any new SPI transfers and wait for outstanding ones */
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> + applespi->cancel_spi = true;
[Severity: High]
This is a pre-existing issue, but does the driver fail to clear the
cancel_spi flag during resume?
When the system suspends, applespi_drain_writes() sets
applespi->cancel_spi = true. Upon waking up, applespi_resume() executes a
state restoration block but appears to omit resetting this flag:
applespi_resume() {
...
scoped_guard(spinlock_irqsave, &applespi->cmd_msg_lock) {
applespi->drain = false;
applespi->have_cl_led_on = false;
applespi->have_bl_level = 0;
applespi->cmd_msg_queued = 0;
applespi->read_active = false;
applespi->write_active = false;
applespi->suspended = false;
}
...
}
Because of this, subsequent calls to applespi_async() will reject all new
SPI transfers and return -ESHUTDOWN, leaving the keyboard and touchpad
permanently unresponsive after the first suspend cycle.
Could cancel_spi be reset to false in applespi_resume() to avoid this?
> + ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> + !applespi_async_outstanding(applespi),
> + applespi->cmd_msg_lock,
> + msecs_to_jiffies(3000));
[Severity: Medium]
Is there an API mismatch between spin_lock_irqsave() and the lock variants
used inside the wait macros?
applespi_remove() acquires the lock using spin_lock_irqsave(). Inside this
critical section, it calls wait_event_lock_irq_timeout().
This macro internally drops the lock using spin_unlock_irq(), which
unconditionally enables interrupts, and then reacquires it with
spin_lock_irq(). This completely overrides the interrupt state saved in the
flags variable.
Should this code just use spin_lock_irq() directly to avoid the mismatch?
> + if (!ret && applespi_async_outstanding(applespi)) {
> + dev_warn(&applespi->spi->dev,
> + "Timed out waiting for SPI transfers to drain, waiting unconditionally\n");
> + wait_event_lock_irq(applespi->wait_queue,
> + !applespi_async_outstanding(applespi),
> + applespi->cmd_msg_lock);
> + }
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>
> debugfs_remove_recursive(applespi->debugfs_root);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720162219.32324-1-fourdollars@debian.org?part=5
^ permalink raw reply
* Re: [PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight
From: sashiko-bot @ 2026-07-20 16:36 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260720162219.32324-3-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Teardown threads wake up prematurely and free `applespi` while `applespi_async_complete` is still running, causing a Use-After-Free.
- [High] Consecutive SPI transfers fail with -EBUSY due to occupied tracking slots, dropping commands and stalling the driver.
- [High] The driver becomes completely broken after a suspend/resume cycle because `cancel_spi` is never reset.
Pre-existing issues:
- [High] Missing cancellation of `applespi->work` in `applespi_remove()` leads to a Use-After-Free.
--
--- Patch [2]: [PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight ---
commit 461be9b81e79a5a61b76018d2f876c28cc39167f
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - track asynchronous SPI transfers in flight
The driver queues read and write packets asynchronously. When shutting
down, removing, or suspending, the driver must guarantee that no
asynchronous transfers remain in flight to prevent memory corruption or
use-after-free conditions. This commit introduces a tracking mechanism
for these async transfers using a spi_complete slot array.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index c501bf7e517e4..c9bbceaf06718 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -607,13 +613,73 @@ static void applespi_setup_write_txfrs(struct applespi_data *applespi)
> spi_message_add_tail(st_t, msg);
> }
>
> +static bool applespi_async_outstanding(struct applespi_data *applespi)
> +{
> + return applespi->spi_complete[0].complete ||
> + applespi->spi_complete[1].complete;
> +}
> +
> +static void applespi_async_complete(void *context)
> +{
> + struct applespi_complete_info *info = context;
> + struct applespi_data *applespi = info->applespi;
> + void (*complete)(void *context);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> + complete = info->complete;
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> + if (complete)
> + complete(applespi);
[Severity: High]
If the inner complete callback queues a new write command, will
applespi_async() see the first slot as still occupied because info->complete
has not been cleared yet?
If a read transfer happens to be active in the other slot, could this cause
applespi_async() to return -EBUSY and drop the new command, permanently
stalling driver initialization?
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> + info->complete = NULL;
> +
> + if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
> + wake_up_all(&applespi->wait_queue);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
[Severity: High]
Can this sequence cause a use-after-free regression?
If applespi_drain_writes() is waiting for write_active to become false:
CPU1 running applespi_async_complete():
complete(applespi);
The inner callback sets write_active = false and wakes wait_queue.
CPU2 running applespi_drain_writes():
wakes up, finishes applespi_remove(), and frees the applespi structure
Could CPU1 then try to acquire the spinlock and clear info->complete on the
freed applespi structure?
[ ... ]
> @@ -1799,6 +1865,7 @@ static void applespi_drain_writes(struct applespi_data *applespi)
> spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
>
> applespi->drain = true;
> + applespi->cancel_spi = true;
> ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> !applespi->write_active,
> applespi->cmd_msg_lock,
[Severity: High]
If applespi_drain_writes() sets cancel_spi to true during applespi_suspend(),
is it ever reset to false in applespi_resume()?
Could this cause applespi_async() to permanently reject new transfers with
-ESHUTDOWN after the system resumes?
[Severity: High]
This isn't a bug introduced by this patch, but while looking at the driver
teardown path, does applespi_remove() need to call
cancel_work_sync(&applespi->work)?
If applespi->work is queued to register the touchpad and the module is
unbound, could devres free the applespi structure before applespi_worker()
executes, resulting in a use-after-free regression?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720162219.32324-1-fourdollars@debian.org?part=2
^ permalink raw reply
* [PATCH v3 5/5] Input: applespi - fix use-after-free in applespi_remove()
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
In-Reply-To: <20260720162219.32324-1-fourdollars@debian.org>
Replace the separate read and write drain helpers with a single barrier using
cancel_spi and wait_event_lock_irq_timeout(). Wait for outstanding asynchronous
SPI operations to complete before tearing down the driver context.
Disable GPE and remove the GPE handler prior to setting cancel_spi = true.
This prevents level-triggered GPE interrupt storms where applespi_notify()
repeatedly attempts applespi_async() and gets rejected with -ESHUTDOWN while
the hardware interrupt line remains asserted.
If the 3-second wait times out while SPI transfers are still outstanding, issue
a warning and fallback to waiting unconditionally until all transfers complete,
guaranteeing that applespi_remove() never returns prematurely while transfers
are active.
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/input/keyboard/applespi.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index a21e89f30387..602780842124 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1939,14 +1939,29 @@ static void applespi_drain_reads(struct applespi_data *applespi)
static void applespi_remove(struct spi_device *spi)
{
struct applespi_data *applespi = spi_get_drvdata(spi);
+ unsigned long flags;
+ long ret;
- applespi_drain_writes(applespi);
-
+ /* Disable GPE and remove handler first to prevent interrupt storm */
acpi_disable_gpe(NULL, applespi->gpe);
acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
device_wakeup_disable(&spi->dev);
- applespi_drain_reads(applespi);
+ /* Prevent any new SPI transfers and wait for outstanding ones */
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ applespi->cancel_spi = true;
+ ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+ !applespi_async_outstanding(applespi),
+ applespi->cmd_msg_lock,
+ msecs_to_jiffies(3000));
+ if (!ret && applespi_async_outstanding(applespi)) {
+ dev_warn(&applespi->spi->dev,
+ "Timed out waiting for SPI transfers to drain, waiting unconditionally\n");
+ wait_event_lock_irq(applespi->wait_queue,
+ !applespi_async_outstanding(applespi),
+ applespi->cmd_msg_lock);
+ }
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
debugfs_remove_recursive(applespi->debugfs_root);
}
--
2.39.5
^ permalink raw reply related
* [PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
In-Reply-To: <20260720162219.32324-1-fourdollars@debian.org>
Set probe_type to PROBE_PREFER_ASYNCHRONOUS to allow the driver core
to run applespi_probe() asynchronously. This improves system boot speeds by
avoiding blocking the main kernel thread during the 3-second touchpad
detection wait.
Additionally, clean up applespi_tp_dim_open() by simplifying product ID
retrieval and avoiding dereferencing touchpad_input_dev without a helper
variable.
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/input/keyboard/applespi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index 6db4eeca3060..a21e89f30387 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1029,6 +1029,7 @@ static void applespi_debug_update_dimensions(struct applespi_data *applespi,
static int applespi_tp_dim_open(struct inode *inode, struct file *file)
{
struct applespi_data *applespi = inode->i_private;
+ struct input_dev *touchpad = applespi->touchpad_input_dev;
if (!applespi->touchpad_input_dev)
return -ENODEV;
@@ -1037,7 +1038,7 @@ static int applespi_tp_dim_open(struct inode *inode, struct file *file)
snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
"0x%.4x %dx%d+%u+%u\n",
- applespi->touchpad_input_dev->id.product,
+ touchpad->id.product,
applespi->tp_dim_min_x, applespi->tp_dim_min_y,
applespi->tp_dim_max_x - applespi->tp_dim_min_x,
applespi->tp_dim_max_y - applespi->tp_dim_min_y);
@@ -2044,6 +2045,7 @@ static struct spi_driver applespi_driver = {
.name = "applespi",
.acpi_match_table = applespi_acpi_match,
.pm = pm_sleep_ptr(&applespi_pm_ops),
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
},
.probe = applespi_probe,
.remove = applespi_remove,
--
2.39.5
^ permalink raw reply related
* [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
In-Reply-To: <20260720162219.32324-1-fourdollars@debian.org>
Consolidate wait queues by renaming drain_complete to wait_queue and use
wait_event_lock_irq_timeout() with a 3-second timeout during read and write
drains to prevent indefinite lockups during driver unbind or PM transitions
if the hardware becomes unresponsive.
If a timeout occurs while a read or write transfer is still active, issue
a warning and fallback to waiting unconditionally until the transfer completion
callback clears the active flag. This guarantees that applespi_drain_writes()
and applespi_drain_reads() never return prematurely while transfers are in flight,
preventing use-after-free (UAF) and DMA memory corruption when devres frees
the driver context and DMA buffers during unbind.
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/input/keyboard/applespi.c | 48 ++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 10 deletions(-)
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index b5ff71cd5a70..c501bf7e517e 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -417,7 +417,7 @@ struct applespi_data {
bool suspended;
bool drain;
- wait_queue_head_t drain_complete;
+ wait_queue_head_t wait_queue;
bool read_active;
bool write_active;
@@ -677,7 +677,7 @@ static int applespi_setup_spi(struct applespi_data *applespi)
return sts;
spin_lock_init(&applespi->cmd_msg_lock);
- init_waitqueue_head(&applespi->drain_complete);
+ init_waitqueue_head(&applespi->wait_queue);
return 0;
}
@@ -725,7 +725,7 @@ static void applespi_msg_complete(struct applespi_data *applespi,
applespi->write_active = false;
if (applespi->drain && !applespi->write_active)
- wake_up_all(&applespi->drain_complete);
+ wake_up_all(&applespi->wait_queue);
if (is_write_msg) {
applespi->cmd_msg_queued = 0;
@@ -1415,7 +1415,7 @@ static void applespi_got_data(struct applespi_data *applespi)
applespi->read_active = false;
applespi->write_active = false;
- wake_up_all(&applespi->drain_complete);
+ wake_up_all(&applespi->wait_queue);
}
return;
@@ -1793,21 +1793,49 @@ static int applespi_probe(struct spi_device *spi)
static void applespi_drain_writes(struct applespi_data *applespi)
{
- guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+ unsigned long flags;
+ long ret;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
applespi->drain = true;
- wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
- applespi->cmd_msg_lock);
+ ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+ !applespi->write_active,
+ applespi->cmd_msg_lock,
+ msecs_to_jiffies(3000));
+ if (!ret && applespi->write_active) {
+ dev_warn(&applespi->spi->dev,
+ "Timed out waiting for write drain, waiting unconditionally\n");
+ wait_event_lock_irq(applespi->wait_queue,
+ !applespi->write_active,
+ applespi->cmd_msg_lock);
+ }
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
}
static void applespi_drain_reads(struct applespi_data *applespi)
{
- guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+ unsigned long flags;
+ long ret;
- wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
- applespi->cmd_msg_lock);
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+ !applespi->read_active,
+ applespi->cmd_msg_lock,
+ msecs_to_jiffies(3000));
+ if (!ret && applespi->read_active) {
+ dev_warn(&applespi->spi->dev,
+ "Timed out waiting for read drain, waiting unconditionally\n");
+ wait_event_lock_irq(applespi->wait_queue,
+ !applespi->read_active,
+ applespi->cmd_msg_lock);
+ }
applespi->suspended = true;
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
}
static void applespi_remove(struct spi_device *spi)
--
2.39.5
^ permalink raw reply related
* [PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
In-Reply-To: <20260720162219.32324-1-fourdollars@debian.org>
The driver queues read and write packets asynchronously. When shutting
down, removing, or suspending, the driver must guarantee that no
asynchronous transfers remain in flight to prevent memory corruption or
use-after-free conditions.
Introduce a 'spi_complete' slot tracking array in struct applespi_data
to represent the two concurrent transfers (one for reads, one for
writes). Implement applespi_async_outstanding() and
applespi_async_complete() to track transfers under cmd_msg_lock.
Set applespi->cancel_spi = true during drain operations to reject new
asynchronous transfer requests with -ESHUTDOWN and activate completion
tracking.
In applespi_async_complete(), invoke the inner completion callback BEFORE
clearing the tracking slot and waking waiters on wait_queue. This ensures
that teardown threads waiting for in-flight transfers to drain cannot wake up
and free the driver structure before the completion callback finishes execution,
preventing use-after-free conditions.
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/input/keyboard/applespi.c | 75 +++++++++++++++++++++++++++++--
1 file changed, 72 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index c501bf7e517e..c9bbceaf0671 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -421,6 +421,12 @@ struct applespi_data {
bool read_active;
bool write_active;
+ struct applespi_complete_info {
+ void (*complete)(void *context);
+ struct applespi_data *applespi;
+ } spi_complete[2];
+ bool cancel_spi;
+
struct work_struct work;
struct touchpad_info_protocol rcvd_tp_info;
@@ -607,13 +613,73 @@ static void applespi_setup_write_txfrs(struct applespi_data *applespi)
spi_message_add_tail(st_t, msg);
}
+static bool applespi_async_outstanding(struct applespi_data *applespi)
+{
+ return applespi->spi_complete[0].complete ||
+ applespi->spi_complete[1].complete;
+}
+
+static void applespi_async_complete(void *context)
+{
+ struct applespi_complete_info *info = context;
+ struct applespi_data *applespi = info->applespi;
+ void (*complete)(void *context);
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ complete = info->complete;
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ if (complete)
+ complete(applespi);
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ info->complete = NULL;
+
+ if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
+ wake_up_all(&applespi->wait_queue);
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
static int applespi_async(struct applespi_data *applespi,
struct spi_message *message, void (*complete)(void *))
{
- message->complete = complete;
- message->context = applespi;
+ struct applespi_complete_info *info;
+ int sts;
+
+ assert_spin_locked(&applespi->cmd_msg_lock);
+
+ if (applespi->cancel_spi) {
+ if (!applespi_async_outstanding(applespi))
+ wake_up_all(&applespi->wait_queue);
+ return -ESHUTDOWN;
+ }
+
+ /*
+ * There can only be at most 2 spi requests in flight, one for "reads"
+ * and one for "writes".
+ */
+ if (!applespi->spi_complete[0].complete)
+ info = &applespi->spi_complete[0];
+ else if (!applespi->spi_complete[1].complete)
+ info = &applespi->spi_complete[1];
+ else {
+ dev_warn(&applespi->spi->dev, "Both SPI async slots in use\n");
+ return -EBUSY;
+ }
+
+ info->complete = complete;
+ info->applespi = applespi;
- return spi_async(applespi->spi, message);
+ message->complete = applespi_async_complete;
+ message->context = info;
+
+ sts = spi_async(applespi->spi, message);
+ if (sts)
+ info->complete = NULL;
+
+ return sts;
}
static inline bool applespi_check_write_status(struct applespi_data *applespi,
@@ -1799,6 +1865,7 @@ static void applespi_drain_writes(struct applespi_data *applespi)
spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
applespi->drain = true;
+ applespi->cancel_spi = true;
ret = wait_event_lock_irq_timeout(applespi->wait_queue,
!applespi->write_active,
applespi->cmd_msg_lock,
@@ -1821,6 +1888,8 @@ static void applespi_drain_reads(struct applespi_data *applespi)
spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ applespi->cancel_spi = true;
+
ret = wait_event_lock_irq_timeout(applespi->wait_queue,
!applespi->read_active,
applespi->cmd_msg_lock,
--
2.39.5
^ permalink raw reply related
* [PATCH v3 3/5] Input: applespi - register touchpad synchronously in probe
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
In-Reply-To: <20260720162219.32324-1-fourdollars@debian.org>
Replace the asynchronous worker for touchpad registration with synchronous
registration during driver probe. Wait up to 3 seconds for the device info
packet and fallback to keyboard-only mode if a timeout occurs.
Prevent a kernel panic in debugfs by checking for a NULL touchpad_input_dev
in applespi_tp_dim_open() before dereferencing it, returning -ENODEV if the
device is operating in keyboard-only mode.
Protect rcvd_tp_info from data races by creating a local copy under
cmd_msg_lock before passing it to applespi_register_touchpad_device().
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
drivers/input/keyboard/applespi.c | 60 ++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 16 deletions(-)
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index c9bbceaf0671..6db4eeca3060 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -427,7 +427,7 @@ struct applespi_data {
} spi_complete[2];
bool cancel_spi;
- struct work_struct work;
+ bool have_tp_info;
struct touchpad_info_protocol rcvd_tp_info;
struct dentry *debugfs_root;
@@ -1030,6 +1030,9 @@ static int applespi_tp_dim_open(struct inode *inode, struct file *file)
{
struct applespi_data *applespi = inode->i_private;
+ if (!applespi->touchpad_input_dev)
+ return -ENODEV;
+
file->private_data = applespi;
snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
@@ -1390,26 +1393,20 @@ applespi_register_touchpad_device(struct applespi_data *applespi,
return 0;
}
-static void applespi_worker(struct work_struct *work)
-{
- struct applespi_data *applespi =
- container_of(work, struct applespi_data, work);
-
- applespi_register_touchpad_device(applespi, &applespi->rcvd_tp_info);
-}
-
static void applespi_handle_cmd_response(struct applespi_data *applespi,
struct spi_packet *packet,
struct message *message)
{
+ unsigned long flags;
+
if (packet->device == PACKET_DEV_INFO &&
le16_to_cpu(message->type) == 0x1020) {
- /*
- * We're not allowed to sleep here, but registering an input
- * device can sleep.
- */
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
applespi->rcvd_tp_info = message->tp_info;
- schedule_work(&applespi->work);
+ applespi->have_tp_info = true;
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ wake_up_all(&applespi->wait_queue);
return;
}
@@ -1677,6 +1674,7 @@ static int applespi_probe(struct spi_device *spi)
acpi_handle spi_handle = ACPI_HANDLE(&spi->dev);
acpi_status acpi_sts;
int sts, i;
+ unsigned long flags;
unsigned long long gpe, usb_status;
/* check if the USB interface is present and enabled already */
@@ -1694,8 +1692,6 @@ static int applespi_probe(struct spi_device *spi)
applespi->spi = spi;
- INIT_WORK(&applespi->work, applespi_worker);
-
/* store the driver data */
spi_set_drvdata(spi, applespi);
@@ -1823,6 +1819,25 @@ static int applespi_probe(struct spi_device *spi)
/* trigger touchpad setup */
applespi_init(applespi, false);
+ /* set up the touchpad as a separate input device if info is received */
+ sts = wait_event_timeout(applespi->wait_queue,
+ READ_ONCE(applespi->have_tp_info),
+ msecs_to_jiffies(3000));
+ if (!sts) {
+ dev_warn(&applespi->spi->dev,
+ "Timed out waiting for touchpad info, continuing keyboard-only\n");
+ } else {
+ struct touchpad_info_protocol tp_info;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ tp_info = applespi->rcvd_tp_info;
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ sts = applespi_register_touchpad_device(applespi, &tp_info);
+ if (sts)
+ goto cancel_spi;
+ }
+
/*
* By default this device is not enabled for wakeup; but USB keyboards
* generally are, so the expectation is that by default the keyboard
@@ -1855,6 +1870,19 @@ static int applespi_probe(struct spi_device *spi)
&applespi_tp_dim_fops);
return 0;
+
+cancel_spi:
+ acpi_disable_gpe(NULL, applespi->gpe);
+ acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+ applespi->cancel_spi = true;
+ wait_event_lock_irq(applespi->wait_queue,
+ !applespi_async_outstanding(applespi),
+ applespi->cmd_msg_lock);
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ return sts;
}
static void applespi_drain_writes(struct applespi_data *applespi)
--
2.39.5
^ permalink raw reply related
* [PATCH v3 0/5] Input: applespi - Fix probe timeout and use-after-free bugs
From: Shih-Yuan Lee @ 2026-07-20 16:22 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mark Brown, linux-input, linux-kernel, Shih-Yuan Lee
This 5-patch series refactors the applespi driver for Apple SPI keyboards and touchpads,
eliminating use-after-free (UAF) race conditions during driver unbind, kernel panics
in debugfs, level-triggered ACPI GPE interrupt storms, and data races.
Specifically, this series:
- Replaces separate drain helpers with a unified wait queue and 3-second timeout barrier,
falling back to unconditional completion waiting if transfers remain active to prevent
premature driver unbind and DMA memory corruption.
- Tracks in-flight asynchronous SPI transfers using a completion tracking array and
cancel_spi flag, ensuring inner completion callbacks execute prior to waking wait
queue waiters to guarantee teardown safety.
- Converts touchpad detection and registration from an asynchronous workqueue worker
to a synchronous probe sequence, while protecting against NULL pointer dereferences in
debugfs during keyboard-only fallback mode.
- Corrects driver unbind sequence by disabling ACPI GPE handlers before marking the driver
as shutting down, avoiding level-triggered interrupt storms on unhandled reads.
Changes in v3:
- Addressed feedback from Sashiko review on the v2 patchset:
- Fixed Use-After-Free in applespi_drain_writes() and applespi_drain_reads()
on timeout by falling back to unconditional wait (wait_event_lock_irq()) if
transfers are still active upon 3-second timeout expiry.
- Activated cancel_spi tracking flag by setting cancel_spi = true in drain functions
under cmd_msg_lock to reject new async requests (-ESHUTDOWN) and trigger wait
queue wakeups.
- Fixed completion callback race in applespi_async_complete() by invoking inner
complete(applespi) callback BEFORE clearing tracking slot and waking waiters
on wait_queue.
- Fixed NULL pointer dereference in applespi_tp_dim_open() when touchpad registration
fails or times out, returning -ENODEV if touchpad_input_dev is NULL.
- Protected rcvd_tp_info from data races in probe by making a safe local copy under
cmd_msg_lock before passing to applespi_register_touchpad_device().
- Fixed GPE Interrupt Storm during unbind in applespi_remove() by disabling and
removing GPE handler BEFORE setting cancel_spi = true and waiting for transfers to drain.
Changes since v1:
- Split the large touchpad registration patch into 4 distinct, single-purpose
commits (wait queue consolidation & timeouts, async queue slots tracking,
synchronous registration, and async probe preference) for better readability.
- Fixed a self-deadlock in applespi_async() where it attempted to acquire
cmd_msg_lock while already held by applespi_notify() and other callers.
- Updated applespi_async() to assert the lock is held by the caller.
- Protected touchpad info flags under cmd_msg_lock in response handling.
- Simplified applespi_tp_dim_open() by removing redundant NULL checks.
- Overhauled GPE disabling and teardown order in applespi_remove() to prevent
GPE interrupt storms and unbind deadlocks.
- Addressed reviewer feedback from Sashiko.
Shih-Yuan Lee (5):
Input: applespi - use unified wait queue with timeouts for drain
Input: applespi - track asynchronous SPI transfers in flight
Input: applespi - register touchpad synchronously in probe
Input: applespi - prefer asynchronous driver probing
Input: applespi - fix use-after-free in applespi_remove()
drivers/input/keyboard/applespi.c | 208 +++++++++++++++++++++++++-----
1 file changed, 175 insertions(+), 33 deletions(-)
--
2.39.5
^ permalink raw reply
* Re: [PATCH v2 3/3] dt-bindings: leds: backlight: Convert TPS65217 to DT schema
From: Eddie @ 2026-07-20 14:07 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Aaro Koskinen, Andreas Kemnade, Conor Dooley, Daniel Thompson,
devicetree, Dmitry Torokhov, dri-devel, Jingoo Han, Kevin Hilman,
Krzysztof Kozlowski, Lee Jones, linux-input, linux-kernel,
linux-leds, linux-omap, Pavel Machek, Rob Herring, Roger Quadros,
Tony Lindgren, daniel.baluta, simona.toaca, goledhruva,
m-chawdhry
In-Reply-To: <20260720-huge-sturgeon-from-shambhala-a1bdcf@quoll>
On Mon, Jul 20, 2026 at 09:54, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sat, Jul 18, 2026 at 09:56:32PM +0000, Eduard Bostina wrote:
> +
> > + regulator-compatible:
> > + $ref: /schemas/types.yaml#/definitions/string
>
> You should not need this property, it is already in regulator.yaml.
I did not find it in regulator.yaml, or in any other schema, but I
found out that it has been deprecated in 2012 by commit 13511def87b9
("regulator: deprecate regulator-compatible DT property") and was
never carried over during the conversion.
I agree it should not be in a new binding, so I will drop it.
It needs a dts change too, tps65217.dtsi names the children
regulator@0..regulator@6, while the driver matches dcdc1, dcdc2,
dcdc3, ldo1…ldo4. of_regulator_match() looks at regulator-compatible
first and falls back to the node name, so without the property nothing
matches and the regulators stop probing.
So I will rename the nodes to dcdc1..ldo4 and drop
regulator-compatible, reg and the address cells.
I will send that as a separate patch in v3 along with the other fixes.
Thanks,
Eduard
^ permalink raw reply
* Re: [PATCH v6 1/4] HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration
From: Lee Jones @ 2026-07-20 12:21 UTC (permalink / raw)
To: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Aaron Skomra, Dmitry Torokhov, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <20260716090726.2711293-1-lee@kernel.org>
Is Jason back in this week?
Note that all of the Sashiko review comments are pre-existing issues.
> Input subsystem guidelines require that device capabilities are advertised
> before the input device is registered. The Wacom driver was violating
> this by advertising the SW_MUTE_DEVICE capability post-registration in
> wacom_set_shared_values() (and duplicating it in device-specific setup
> cases).
>
> Resolve this by moving the SW_MUTE_DEVICE capability setup to
> wacom_setup_touch_input_capabilities() for all touch devices that support
> it, excluding TabletPC devices.
>
> Additionally, replace the lookup-dependent
> 'wacom_wac->shared->touch->product' references with 'hdev->product'
> inside wacom_setup_touch_input_capabilities() as 'hdev' is already
> available and represents the touch device itself.
>
> Fixes: d2ec58aee8b1 ("HID: wacom: generic: support generic touch switch")
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>
> v4 -> v5: New patch used to split out SW_MUTE_DEVICE as per Jason's request
> v5 -> v6: Unconditionally advertise SW_MUTE_DEVICE on generic touch devices
>
> drivers/hid/wacom_sys.c | 6 ------
> drivers/hid/wacom_wac.c | 19 +++++++++++--------
> 2 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0eafa483b7f7..6c2a3ddfe366 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2359,12 +2359,6 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
> wacom_wac->shared->is_touch_on = true;
> }
>
> - if (wacom_wac->shared->has_mute_touch_switch &&
> - wacom_wac->shared->touch_input) {
> - set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
> - input_set_capability(wacom_wac->shared->touch_input, EV_SW,
> - SW_MUTE_DEVICE);
> - }
> }
>
> static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a29bf051ada7..32c4fd03479e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3953,6 +3953,8 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> struct wacom_wac *wacom_wac)
> {
> + struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> + struct hid_device *hdev = wacom->hdev;
> struct wacom_features *features = &wacom_wac->features;
>
> if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
> @@ -3963,9 +3965,12 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> else
> __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>
> - if (features->type == HID_GENERIC)
> + if (features->type == HID_GENERIC) {
> + input_dev->evbit[0] |= BIT_MASK(EV_SW);
> + __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> /* setup has already been done */
> return 0;
> + }
>
> input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> __set_bit(BTN_TOUCH, input_dev->keybit);
> @@ -3997,19 +4002,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> input_dev->evbit[0] |= BIT_MASK(EV_SW);
> __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>
> - if (wacom_wac->shared->touch->product == 0x361) {
> + if (hdev->product == 0x361) {
> input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> 0, 12440, 4, 0);
> input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> 0, 8640, 4, 0);
> - }
> - else if (wacom_wac->shared->touch->product == 0x360) {
> + } else if (hdev->product == 0x360) {
> input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> 0, 8960, 4, 0);
> input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> 0, 5920, 4, 0);
> - }
> - else if (wacom_wac->shared->touch->product == 0x393) {
> + } else if (hdev->product == 0x393) {
> input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> 0, 6400, 4, 0);
> input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> @@ -4039,8 +4042,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> fallthrough;
>
> case WACOM_27QHDT:
> - if (wacom_wac->shared->touch->product == 0x32C ||
> - wacom_wac->shared->touch->product == 0xF6) {
> + if (hdev->product == 0x32C ||
> + hdev->product == 0xF6) {
> input_dev->evbit[0] |= BIT_MASK(EV_SW);
> __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> wacom_wac->has_mute_touch_switch = true;
> --
> 2.55.0.141.g00534a21ce-goog
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v2] Input: iforce - validate input packet lengths
From: sashiko-bot @ 2026-07-20 12:12 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260720115018.75045-1-pengpeng@iscas.ac.cn>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Data race on core_effect->flags due to non-atomic __test_and_set_bit.
- [Medium] Missing input_sync() after reporting force-feedback status (EV_FF_STATUS) events.
--
--- Patch [1]: [PATCH v2] Input: iforce - validate input packet lengths ---
commit 32891702a73654a7ce9748db1e4baa8f0cbb9362
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Input: iforce - validate input packet lengths
iforce_process_packet() reads fixed fields from joystick, wheel and status
packets without first checking their lengths. In particular, the shared
hats-and-buttons helper unconditionally reads data[6]. The status tail is
a sequence of 16-bit effect addresses, but an incomplete final address is
also consumed. A successful zero-length USB URB additionally reads the
packet ID before the common parser is called.
Reject the zero-length USB transfer, require the seven-byte joystick and
wheel prefixes and the two-byte status prefix, and consume only complete
status-tail addresses.
> diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c
> index effa76bfd8f9a..e8a34d68a51f2 100644
> --- a/drivers/input/joystick/iforce/iforce-packets.c
> +++ b/drivers/input/joystick/iforce/iforce-packets.c
[ ... ]
> @@ -200,7 +209,7 @@ void iforce_process_packet(struct iforce *iforce,
> }
> }
[Severity: Medium]
This is a pre-existing issue, but is there a missing input_sync() after the
input_report_ff_status() calls just above this code?
The input subsystem guidelines require that every logical group of events must
be followed by input_sync(), or the events stay in the buffer. While there is
an input_sync() earlier for BTN_DEAD, the EV_FF_STATUS events emitted here
might remain buffered and not reach userspace until a subsequent event
triggers another sync.
> - for (j = 3; j < len; j += 2)
> + for (j = 3; j + sizeof(u16) <= len; j += 2)
> mark_core_as_ready(iforce, get_unaligned_le16(data + j));
[Severity: High]
This isn't a bug introduced by this patch, but does the call to
mark_core_as_ready() here expose a data race with iforce_upload_effect()?
The interrupt completion handler calls mark_core_as_ready(), which uses atomic
bit operations on core_effects[i].flags:
drivers/input/joystick/iforce/iforce-packets.c:mark_core_as_ready() {
...
clear_bit(FF_CORE_UPDATE, iforce->core_effects[i].flags);
...
}
However, iforce_upload_effect() in iforce-main.c executes in process context
with ff->mutex held (which does not disable interrupts) and uses a non-atomic
read-modify-write:
drivers/input/joystick/iforce/iforce-main.c:iforce_upload_effect() {
...
if (__test_and_set_bit(FF_CORE_IS_USED, core_effect->flags)) {
...
}
If the interrupt executes between the read and write phases of
__test_and_set_bit(), the interrupt's modification could be overwritten.
Could this potentially leave FF_CORE_UPDATE stuck at 1 and render the effect
impossible to update again?
> break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720115018.75045-1-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply
* Re: [PATCH 02/10] iio: gyro: hid-sensor-gyro-3d: use local struct device
From: Andy Shevchenko @ 2026-07-20 11:54 UTC (permalink / raw)
To: sanjayembeddedse
Cc: Jiri Kosina, Jonathan Cameron, Srinivas Pandruvada, David Lechner,
Nuno Sá, Andy Shevchenko, linux-input, linux-iio,
linux-kernel
In-Reply-To: <20260720-hid-iio-local-struct-device-v1-2-74496fee6327@gmail.com>
On Mon, Jul 20, 2026 at 03:10:14PM +0530, Sanjay Chitroda via B4 Relay wrote:
> Introduce a local struct device pointer derived from &pdev->dev.
> This avoids repeated &pdev->dev usage and improves readability.
...
> if (ret) {
> - dev_err(&pdev->dev, "failed to setup common attributes\n");
> + dev_err(dev, "failed to setup common attributes\n");
> return ret;
Nope, First one (to eliminate a lot of churn) should be to convert this and
similar to use
return dev_err_probe(...);
> }
>
> - indio_dev->channels = devm_kmemdup(&pdev->dev, gyro_3d_channels,
> + indio_dev->channels = devm_kmemdup(dev, gyro_3d_channels,
> sizeof(gyro_3d_channels), GFP_KERNEL);
> if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> + dev_err(dev, "failed to duplicate channels\n");
And this kind of messages are dead code and must be just removed completely.
> return -ENOMEM;
> }
...
Please, revisit the whole series.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2] Input: iforce - validate input packet lengths
From: Pengpeng Hou @ 2026-07-20 11:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Bryam Vargas, linux-input, linux-kernel, Pengpeng Hou
iforce_process_packet() reads fixed fields from joystick, wheel and status
packets without first checking their lengths. In particular, the shared
hats-and-buttons helper unconditionally reads data[6]. The status tail is
a sequence of 16-bit effect addresses, but an incomplete final address is
also consumed. A successful zero-length USB URB additionally reads the
packet ID before the common parser is called.
Reject the zero-length USB transfer, require the seven-byte joystick and
wheel prefixes and the two-byte status prefix, and consume only complete
status-tail addresses.
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260706092016.78176-1-pengpeng@iscas.ac.cn/
- cover the data[6] access in the shared hats-and-buttons helper
- cover zero-length USB completions and incomplete status-tail u16 values
- rebase onto v7.2-rc4
drivers/input/joystick/iforce/iforce-packets.c | 11 ++++++++++-
drivers/input/joystick/iforce/iforce-usb.c | 3 +++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c
index effa76bfd8f9..e8a34d68a51f 100644
--- a/drivers/input/joystick/iforce/iforce-packets.c
+++ b/drivers/input/joystick/iforce/iforce-packets.c
@@ -155,6 +155,9 @@ void iforce_process_packet(struct iforce *iforce,
switch (packet_id) {
case 0x01: /* joystick position data */
+ if (len < 7)
+ break;
+
input_report_abs(dev, ABS_X,
(__s16) get_unaligned_le16(data));
input_report_abs(dev, ABS_Y,
@@ -170,6 +173,9 @@ void iforce_process_packet(struct iforce *iforce,
break;
case 0x03: /* wheel position data */
+ if (len < 7)
+ break;
+
input_report_abs(dev, ABS_WHEEL,
(__s16) get_unaligned_le16(data));
input_report_abs(dev, ABS_GAS, 255 - data[2]);
@@ -181,6 +187,9 @@ void iforce_process_packet(struct iforce *iforce,
break;
case 0x02: /* status report */
+ if (len < 2)
+ break;
+
input_report_key(dev, BTN_DEAD, data[0] & 0x02);
input_sync(dev);
@@ -200,7 +209,7 @@ void iforce_process_packet(struct iforce *iforce,
}
}
- for (j = 3; j < len; j += 2)
+ for (j = 3; j + sizeof(u16) <= len; j += 2)
mark_core_as_ready(iforce, get_unaligned_le16(data + j));
break;
diff --git a/drivers/input/joystick/iforce/iforce-usb.c b/drivers/input/joystick/iforce/iforce-usb.c
index 0482eaaecf39..f04370e4191e 100644
--- a/drivers/input/joystick/iforce/iforce-usb.c
+++ b/drivers/input/joystick/iforce/iforce-usb.c
@@ -158,6 +158,9 @@ static void iforce_usb_irq(struct urb *urb)
goto exit;
}
+ if (!urb->actual_length)
+ goto exit;
+
iforce_process_packet(iforce, iforce_usb->data_in[0],
iforce_usb->data_in + 1, urb->actual_length - 1);
^ permalink raw reply related
* Re: [PATCH v2 4/5] Input: applespi - prefer asynchronous driver probing
From: sashiko-bot @ 2026-07-20 10:32 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260720101435.13612-5-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Failure to cancel in-flight SPI async transfers upon unbind timeout results in use-after-free in completion callbacks.
Pre-existing issues:
- [High] Kernel panic in `applespi_tp_dim_open` due to an unchecked NULL `touchpad_input_dev` when touchpad initialization times out.
--
--- Patch [4]: [PATCH v2 4/5] Input: applespi - prefer asynchronous driver probing ---
commit df316467951827e13575294cf4f7b16d6876f89a
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - prefer asynchronous driver probing
Set probe_type to PROBE_PREFER_ASYNCHRONOUS to allow the driver core
to run applespi_probe() asynchronously. This improves system boot speeds by
avoiding blocking the main kernel thread during the 3-second touchpad
detection wait.
Additionally, clean up applespi_tp_dim_open() by simplifying product ID
retrieval and avoiding dereferencing touchpad_input_dev without a helper
variable.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1027,12 +1027,13 @@ static void applespi_debug_update_dimensions(struct applespi_data *applespi,
> static int applespi_tp_dim_open(struct inode *inode, struct file *file)
> {
> struct applespi_data *applespi = inode->i_private;
> + struct input_dev *touchpad = applespi->touchpad_input_dev;
>
> file->private_data = applespi;
>
> snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
> "0x%.4x %dx%d+%u+%u\n",
> - applespi->touchpad_input_dev->id.product,
> + touchpad->id.product,
[Severity: High]
This isn't a bug introduced by this patch, but can this dereference a NULL
pointer if the touchpad fails to initialize?
If the 3-second wait in applespi_probe() times out:
applespi_probe() {
...
sts = wait_event_timeout(applespi->wait_queue,
READ_ONCE(applespi->have_tp_info),
msecs_to_jiffies(3000));
...
}
Then applespi->touchpad_input_dev is never allocated. However, the
debugfs file is still created unconditionally later in applespi_probe():
debugfs_create_file("tp_dim", 0400, applespi->debugfs_root, applespi,
&applespi_tp_dim_fops);
If a user subsequently reads this file, applespi_tp_dim_open() is called
and will dereference the unconditionally NULL touchpad pointer when
retrieving the product ID.
> applespi->tp_dim_min_x, applespi->tp_dim_min_y,
> applespi->tp_dim_max_x - applespi->tp_dim_min_x,
> applespi->tp_dim_max_y - applespi->tp_dim_min_y);
[ ... ]
> @@ -2015,6 +2016,7 @@ static struct spi_driver applespi_driver = {
> .name = "applespi",
> .acpi_match_table = applespi_acpi_match,
> .pm = pm_sleep_ptr(&applespi_pm_ops),
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
[Severity: High]
Does the timeout added in this series for unbinding lead to a use-after-free
if SPI transfers are still active?
In applespi_remove(), the driver attempts to drain in-flight operations:
applespi_remove() {
applespi_drain_writes(applespi);
acpi_disable_gpe(NULL, applespi->gpe);
...
applespi_drain_reads(applespi);
...
}
These drain functions wait with a 3-second timeout:
applespi_drain_writes() {
wait_event_lock_irq_timeout(applespi->wait_queue,
!applespi->write_active,
applespi->cmd_msg_lock,
msecs_to_jiffies(3000));
}
If this timeout expires because the hardware is unresponsive, the function
returns and applespi_remove() completes. The devres subsystem then frees
the applespi structure and its DMA-mapped buffers.
When the asynchronous SPI transfer eventually completes, the SPI core invokes
applespi_async_complete() with the freed context pointer:
applespi_async_complete() {
struct applespi_complete_info *info = context;
struct applespi_data *applespi = info->applespi;
...
spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
...
}
Will this dereference the freed applespi object and corrupt memory?
> },
> .probe = applespi_probe,
> .remove = applespi_remove,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720101435.13612-1-fourdollars@debian.org?part=4
^ permalink raw reply
* Re: [PATCH v2 2/5] Input: applespi - track asynchronous SPI transfers in flight
From: sashiko-bot @ 2026-07-20 10:31 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260720101435.13612-3-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] The newly introduced asynchronous SPI transfer tracking logic and `cancel_spi` flag are dead code, failing to prevent use-after-free during teardown.
- [High] The asynchronous tracking mechanism signals completion to potential waiters before the inner callback finishes executing, introducing a Use-After-Free window.
Pre-existing issues:
- [High] The driver schedules `applespi->work` but fails to cancel it during driver removal, leading to a use-after-free.
- [High] Re-queueing the statically allocated `wr_m` SPI message after a timeout corrupts the SPI subsystem's message queue.
- [High] A pre-existing use-after-free vulnerability exists in `applespi_async_read_complete()` during device removal.
--
--- Patch [2]: [PATCH v2 2/5] Input: applespi - track asynchronous SPI transfers in flight ---
commit 0ed887ee2137eeb7173f66b5f712a9622f7a16c3
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - track asynchronous SPI transfers in flight
This patch aims to fix use-after-free conditions during driver teardown by
tracking asynchronous SPI requests. It introduces a completion tracking array
and a cancel_spi flag to represent concurrent read and write transfers.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 64bbeba85ea9e..a8f8d5370e95a 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -421,6 +421,12 @@ struct applespi_data {
> bool read_active;
> bool write_active;
>
> + struct applespi_complete_info {
> + void (*complete)(void *context);
> + struct applespi_data *applespi;
> + } spi_complete[2];
> + bool cancel_spi;
> +
[Severity: High]
Is cancel_spi ever initialized or set to true during driver teardown?
Looking through the patch and the driver, it appears cancel_spi is declared
but never set to true. As a result, the checks for applespi->cancel_spi in
applespi_async() and applespi_async_complete() will always evaluate to false,
leaving the new tracking mechanism inactive.
Additionally, it doesn't look like applespi_remove() was updated to wait for
!applespi_async_outstanding(applespi) during driver removal. Does the
teardown path still proceed without waiting for the tracked transfers to
finish?
[ ... ]
> +static void applespi_async_complete(void *context)
> +{
> + struct applespi_complete_info *info = context;
> + struct applespi_data *applespi = info->applespi;
> + void (*complete)(void *context);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + complete = info->complete;
> + info->complete = NULL;
> +
> + if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
> + wake_up_all(&applespi->wait_queue);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> + if (complete)
> + complete(applespi);
> +}
[Severity: High]
Could waking up the wait queue before invoking the inner completion callback
introduce a use-after-free?
If a teardown thread is waiting on the wait_queue, it will unblock as soon
as wake_up_all() is called. This thread might then finish driver removal and
allow the devm subsystem to free the applespi structure.
If that happens, the subsequent call to complete(applespi) will dereference
the freed applespi object. Should the inner completion be invoked before
waking up the wait queue?
[Severity: High]
This is a pre-existing issue, but does driver removal fail to cancel the
scheduled workqueue?
Looking at applespi_remove():
applespi_remove() {
applespi_drain_writes(applespi);
applespi_drain_reads(applespi);
...
}
Since applespi is managed by devm_kzalloc(), its memory is freed
automatically shortly after applespi_remove() returns. If the touchpad
initialization worker scheduled in applespi->work runs after driver removal,
applespi_worker() will dereference the freed applespi structure.
Should cancel_work_sync(&applespi->work) be called during teardown?
[Severity: High]
This is a pre-existing issue, but can the command timeout logic corrupt the
SPI message queue?
In applespi_send_cmd_msg():
applespi_send_cmd_msg() {
if (applespi->cmd_msg_queued) {
if (ktime_ms_delta(ktime_get(), applespi->cmd_msg_queued) < 1000)
return 0;
applespi->cmd_msg_queued = 0;
applespi->write_active = false;
}
...
sts = applespi_async(applespi, &applespi->wr_m, applespi_async_write_complete);
}
If a command times out, the driver assumes the transfer is no longer in
flight and clears its internal flags. However, the message might still be
pending in the SPI controller's active queue.
Re-queueing the statically allocated applespi->wr_m struct via spi_async()
while it is still linked in the SPI core's lists will invoke list_add_tail()
on an already linked node, which corrupts the list.
[Severity: High]
This is a pre-existing issue, but is there a use-after-free window in the
read completion handler during teardown?
During driver removal, applespi_drain_reads() waits for read_active to
become false:
applespi_drain_reads() {
wait_event_lock_irq_timeout(applespi->wait_queue,
!applespi->read_active, ...);
}
Inside applespi_async_read_complete():
applespi_async_read_complete() {
...
applespi_got_data(applespi);
...
acpi_finish_gpe(NULL, applespi->gpe);
}
The call to applespi_got_data() clears read_active, which wakes up
applespi_drain_reads(). This allows the driver removal thread to finish
and free the applespi object. When the completion handler resumes, it
calls acpi_finish_gpe(NULL, applespi->gpe), dereferencing the freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720101435.13612-1-fourdollars@debian.org?part=2
^ permalink raw reply
* Re: [PATCH v2 5/5] Input: applespi - fix use-after-free in applespi_remove()
From: sashiko-bot @ 2026-07-20 10:30 UTC (permalink / raw)
To: Shih-Yuan Lee; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260720101435.13612-6-fourdollars@debian.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] applespi_async_complete() triggers a use-after-free by signaling completion before actually executing the callback.
- [High] Ignored timeout in `wait_event_lock_irq_timeout` leads to a Use-After-Free on outstanding SPI transfers.
- [High] Setting `cancel_spi = true` before disabling the level-triggered GPE causes an interrupt storm.
--
--- Patch [5]: [PATCH v2 5/5] Input: applespi - fix use-after-free in applespi_remove() ---
commit 8828f1f25e357bbbc7a4945659f2ddf6d10cda5c
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - fix use-after-free in applespi_remove()
This commit replaces the separate read and write drain helpers with a single
barrier using the existing cancel_spi and wait_event_lock_irq mechanism. It
waits for outstanding asynchronous operations to complete before disabling
the GPE and tearing down the device.
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 95a8f790eaffc..088337f060b24 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1910,15 +1910,22 @@ static void applespi_drain_reads(struct applespi_data *applespi)
> static void applespi_remove(struct spi_device *spi)
> {
> struct applespi_data *applespi = spi_get_drvdata(spi);
> + unsigned long flags;
>
> - applespi_drain_writes(applespi);
> + /* Prevent any new SPI transfers and wait for outstanding ones */
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> + applespi->cancel_spi = true;
[Severity: High]
If we set cancel_spi to true while the level-triggered GPE is still enabled,
could this cause an interrupt storm?
When the GPE fires, applespi_notify() attempts to queue a read via
applespi_async(), which immediately rejects it with -ESHUTDOWN. Because the
read is rejected, the trackpad hardware never deasserts the interrupt line.
Would it be safer to disable the GPE before setting this flag?
> + wait_event_lock_irq_timeout(applespi->wait_queue,
> + !applespi_async_outstanding(applespi),
> + applespi->cmd_msg_lock,
> + msecs_to_jiffies(3000));
[Severity: High]
If the wait times out (returning 0), the driver continues tearing down and
returns, allowing devm to free the memory. If the delayed SPI transfer
completes shortly after, won't that result in a use-after-free in the SPI
core or applespi_async_complete() when it accesses the freed driver data?
[Severity: High]
Does this wait condition race with the inner completion callback?
Looking at applespi_async_complete(), it signals completion before executing
the inner callback:
drivers/input/keyboard/applespi.c:applespi_async_complete() {
...
info->complete = NULL;
if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
wake_up_all(&applespi->wait_queue);
spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
if (complete)
complete(applespi);
...
}
If the teardown thread wakes up as soon as applespi_async_outstanding() is
false and frees the device structure, will complete(applespi) dereference
freed memory?
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>
> + /* Disable GPE and remove handler */
> acpi_disable_gpe(NULL, applespi->gpe);
> acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
> device_wakeup_disable(&spi->dev);
>
> - applespi_drain_reads(applespi);
> -
> debugfs_remove_recursive(applespi->debugfs_root);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720101435.13612-1-fourdollars@debian.org?part=5
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox