Re: [PATCH v4 11/11] HID: spi-hid: add panel follower support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Leak of wake IRQ structure on probe failure or driver unbind for panel followers.
- [High] Unbalanced regulator power_down during remove() for unbound panel followers.
- [High] Missing power_state update in spi_hid_dev_init() breaks device and leaks regulator after suspend.

Pre-existing issues:
- [High] Preexisting Use-After-Free due to race between cancel_work_sync() and hid_destroy_device() timeouts.
--

commit 6b1c4605757961c511d63ab9f2108f26f48fd916
Author: Jingyuan Liang <jingyliang@chromium.org>

HID: spi-hid: add panel follower support

Add support to spi-hid to be a panel follower.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 517f069134775..27f25d95ed280 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -1281,6 +1281,106 @@ const struct attribute_group *spi_hid_groups[] = {

[ ... ]

> +static int spi_hid_dev_init(struct spi_hid *shid)
> +{
> +	struct spi_device *spi = shid->spi;
> +	struct device *dev = &spi->dev;
> +	int error;
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +	error = shid->ops->power_up(shid->ops);
> +	if (error) {
> +		dev_err(dev, "%s: could not power up\n", __func__);
> +		shid->regulator_error_count++;
> +		shid->regulator_last_error = error;
> +		return error;
> +	}
> +
> +	shid->ops->deassert_reset(shid->ops);
> +
> +	enable_irq(spi->irq);
> +
> +	return 0;
> +}

[Severity: High]
If a panel follower suspends before it successfully receives its hid_version,
spi_hid_suspend() will legitimately power down the device and set power_state =
HIDSPI_OFF.

Upon resume, spi_hid_panel_follower_work() retries initialization by calling
spi_hid_dev_init(). Since spi_hid_dev_init() does not update power_state to
HIDSPI_ON, does the state remain HIDSPI_OFF?

If so, when the reset response interrupt arrives, would spi_hid_dev_irq() drop
it because it believes the device is off, leaving the device broken?
Furthermore, would any subsequent suspend return early because power_state ==
HIDSPI_OFF, failing to power down the regulator and causing a power leak?

[ ... ]

> +static int spi_hid_register_panel_follower(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +
> +	shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs;
> +
> +	/*
> +	 * If we're not in control of our own power up/power down then we can't
> +	 * do the logic to manage wakeups. Give a warning if a user thought
> +	 * that was possible then force the capability off.
> +	 */
> +	if (device_can_wakeup(dev)) {
> +		dev_warn(dev, "Can't wakeup if following panel\n");
> +		device_set_wakeup_capable(dev, false);
> +	}

[Severity: High]
Does explicitly executing device_set_wakeup_capable(dev, false) mutate the
dev->power.can_wakeup flag to false, which would then break the symmetry of the
wake IRQ cleanup conditions?

In spi_hid_core_probe(), the wake IRQ is successfully registered if
device_may_wakeup(dev) is true. During error cleanup (err_wake_irq) or driver
removal (spi_hid_core_remove()), the code conditionally calls
dev_pm_clear_wake_irq(dev) based on if (device_may_wakeup(dev)).

Since the capability was previously cleared here, will this check evaluate to
false and cause the driver to skip the cleanup, leaking the wake IRQ
registration?

[ ... ]

> @@ -1300,6 +1400,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
>  	shid->ops = ops;
>  	shid->conf = conf;
>  	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +	shid->is_panel_follower = drm_is_panel_follower(&spi->dev);

[Severity: High]
During spi_hid_core_probe(), shid->power_state is unconditionally initialized
to HIDSPI_ON. For a panel follower, the actual hardware initialization and
regulator power_up() are deferred to spi_hid_panel_follower_work().

If the driver is unbound (or probe fails gracefully after device registration)
before the panel prepares, power_up() is never called. However,
spi_hid_core_remove() checks if (shid->power_state != HIDSPI_OFF) and calls
power_down().

Will this trigger a power_down() on an already disabled regulator, resulting in
an unbalanced regulator use-count?

[ ... ]

> @@ -1376,15 +1470,21 @@ void spi_hid_core_remove(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	int error;
>  
> -	disable_irq(spi->irq);
> +	if (shid->is_panel_follower)
> +		drm_panel_remove_follower(&shid->panel_follower);
> +	else
> +		disable_irq(spi->irq);
> +
>  	cancel_work_sync(&shid->reset_work);
>  
>  	spi_hid_stop_hid(shid);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk due to a race
between cancel_work_sync() and hid_destroy_device() timeouts?

In spi_hid_core_remove(), cancel_work_sync(&shid->reset_work) is called prior
to spi_hid_stop_hid(shid), which destroys the HID device. Because
disable_irq(spi->irq) is called beforehand, any concurrent HID request (like a
userspace GET_REPORT ioctl) waiting in
wait_for_completion_interruptible_timeout() will inevitably time out.

Upon timing out, the sync request handler sets the error flag and executes
schedule_work(&shid->reset_work). Since this timeout can occur after
cancel_work_sync() has already finished but before remove() concludes, the
work is freshly queued. The remove() function then frees shid, resulting in
the reset_work executing on freed memory.

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