Re: [PATCH 04/13] platform/apple-gmux: Fix Interrupts for MMIO systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] platform/apple-gmux: incorrect teardown order in gmux_remove() causes UAF
- [High] platform/apple-gmux: cancel_work_sync() on uninitialized work in gmux_remove()
- [High] platform/apple-gmux: self-deadlock on powerchange_done in gmux_notify_work()
- [High] platform/apple-gmux: iounmap() before cancel_work_sync() in gmux_probe()
- [High] platform/apple-gmux: missing interrupt_work cancellation in gmux_suspend()
- [High] platform/apple-gmux: unhandled status=0 GPEs and late init in gmux_probe()
- [High] platform/x86: apple-gmux: unreleased lock and uninit val in gmux_mmio_read8()
- [Medium] platform/apple-gmux: lost events and unbounded loop in gmux_notify_work()
--

commit 8b786eb41ee899344257c35e97176ec39dc10858
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Fix Interrupts for MMIO systems

This commit moves MMIO GMUX interrupt handling to a workqueue item, checks
for failed interrupt status bits, and wakes the discrete GPU via runtime PM
when external display hotplug interrupts occur on MMIO systems.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 076ed7ba6af90..bef87a57ad43c 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -825,23 +838,78 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> -	/* Prevent flood of status=0 interrupts */
> -	if (gmux_data->config == &apple_gmux_mmio)
> -		acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
>  }
>  
> +static void gmux_notify_work(struct work_struct *work)
> +{
> +	struct apple_gmux_data *gmux_data =
> +		container_of(work, struct apple_gmux_data, interrupt_work);
> +	u8 status;
> +	bool turn_on_gpu = false;
> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	if (!status)
> +		return;
> +
> +	gmux_disable_interrupts(gmux_data);
> +	pr_debug("Notify handler called: status %d\n", status);
> +	acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);

[Severity: High]
If a GPE interrupt fires on an MMIO GMUX system while
GMUX_PORT_INTERRUPT_STATUS reads 0, doesn't returning early before calling
GMSP(0) leave the status=0 interrupt flood uncleared?

Previously, gmux_clear_interrupts() called GMSP(0) even when status was 0.
In gmux_probe(), acpi_install_notify_handler() and acpi_enable_gpe() are
called before init_completion(&gmux_data->powerchange_done) and before
gmux_enable_interrupts(gmux_data, 1) initializes the GPIO polarity via
GMSP(0):

gmux_probe()
    acpi_install_notify_handler(..., &gmux_notify_handler, pnp);
    acpi_enable_gpe(NULL, gmux_data->gpe);
    ...
    init_completion(&gmux_data->powerchange_done);
    gmux_enable_interrupts(gmux_data, 1);

If a GPE fires in that window (or during resume before gmux_resume() runs)
with status == 0, gmux_notify_work() returns without calling GMSP(0). And if
a GPE fires in that probe window with GMUX_INTERRUPT_STATUS_POWER set, won't
gmux_notify_work() call complete(&gmux_data->powerchange_done) before
init_completion(&gmux_data->powerchange_done) has initialized the waitqueue?

[Severity: High]
What happens in gmux_notify_work() if gmux_mmio_wait() times out inside
gmux_mmio_read8()?

On MMIO systems, gmux_interrupt_get_status() and gmux_read8() call
gmux_mmio_read8() (introduced in the earlier series patch "platform/apple-gmux:
Fix MMIO interface"), which has the same structure as gmux_mmio_read32():

static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
{
	u8 val;

	mutex_lock(&gmux_data->index_lock);
	if (gmux_mmio_wait(gmux_data)) {
		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
		iowrite8(GMUX_MMIO_READ | sizeof(val),
			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
		if (!gmux_mmio_wait(gmux_data))
			return 0;
		val = ioread8(gmux_data->iomem_base);
	}
	mutex_unlock(&gmux_data->index_lock);

	return val;
}

If the first gmux_mmio_wait() call fails, gmux_mmio_read8() (and
gmux_mmio_read32()) returns the uninitialized stack variable val, causing
gmux_notify_work() to treat stack garbage as the interrupt status and
potentially loop, complete gmux_data->powerchange_done, or wake the discrete
GPU.

If the second gmux_mmio_wait() call fails, gmux_mmio_read8() (and
gmux_mmio_read32()) returns 0 while still holding gmux_data->index_lock, so
the subsequent gmux_enable_interrupts() -> gmux_write8() -> gmux_mmio_write8()
call in gmux_notify_work() will immediately deadlock on gmux_data->index_lock.

> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	while (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		if (status & GMUX_INTERRUPT_STATUS_FAILED) {
> +			pr_warn_ratelimited("interrupt failed: status: 0x%x\n", status);
> +			status = gmux_interrupt_get_status(gmux_data);
> +			continue;
> +		}

[Severity: Medium]
If GMUX_INTERRUPT_STATUS_FAILED is set in status at the same time as
GMUX_INTERRUPT_STATUS_POWER or GMUX_INTERRUPT_STATUS_HOTPLUG, doesn't writing
the full status value to GMUX_PORT_INTERRUPT_STATUS clear all of those bits in
hardware before the continue statement skips handling them?

Also, if clearing GMUX_PORT_INTERRUPT_STATUS fails (for example, if
gmux_mmio_wait() times out in gmux_mmio_write8() or a hardware status bit
remains stuck), could while (status) spin indefinitely on system_percpu_wq?

> +		if (status & GMUX_INTERRUPT_STATUS_POWER)
> +			complete(&gmux_data->powerchange_done);
> +
> +		if (status & GMUX_INTERRUPT_STATUS_HOTPLUG) {
> +			/*
> +			 * We get an interrupt every time a display connects or disconnects, but
> +			 * the GET_DISPLAY port only tells us if a display is connected or not,
> +			 * so we resume every time a display is connected or disconnected.
> +			 */
> +			bool external_state = !!gmux_read8(gmux_data,
> +						GMUX_PORT_SWITCH_GET_DISPLAY);
> +			if (external_state != gmux_data->external_state) {
> +				gmux_data->external_state = external_state;
> +				if (external_state)
> +					pr_debug("A display has been connected\n");
> +				else
> +					pr_debug("All displays has been disconnected\n");
> +			}
> +			if (!gmux_data->external_switchable && gmux_data->discrete_pdev)
> +				turn_on_gpu = true;
> +		}
> +		status = gmux_interrupt_get_status(gmux_data);
> +	}
> +
> +	gmux_enable_interrupts(gmux_data, 0);
> +
> +	if (turn_on_gpu)
> +		pm_runtime_resume(&gmux_data->discrete_pdev->dev);
> +
> +}

[Severity: High]
Won't calling synchronous pm_runtime_resume(&gmux_data->discrete_pdev->dev)
from inside gmux_notify_work() self-deadlock on gmux_data->powerchange_done
when the discrete GPU is runtime-suspended?

When discrete_pdev is runtime-suspended, pm_runtime_resume() synchronously
runs the power switch callback on the current worker thread:

gmux_notify_work() [executing gmux_data->interrupt_work]
  pm_runtime_resume(&gmux_data->discrete_pdev->dev)
    ...
      vga_switcheroo_runtime_resume()
        vga_switcheroo_power_switch()
          gmux_set_power_state()
            gmux_set_discrete_state(apple_gmux_data, VGA_SWITCHEROO_ON)
              reinit_completion(&gmux_data->powerchange_done);
              ...
              wait_for_completion_interruptible_timeout(&gmux_data->powerchange_done,
                                                        msecs_to_jiffies(200));

When the resulting GMUX_INTERRUPT_STATUS_POWER interrupt fires,
gmux_notify_handler() calls schedule_work(&gmux_data->interrupt_work).
Because gmux_data->interrupt_work is the same work_struct currently blocked in
pm_runtime_resume() and a work_struct cannot run concurrently with itself,
gmux_notify_work() cannot run to call complete(&gmux_data->powerchange_done)
until wait_for_completion_interruptible_timeout() times out after 200ms.

Would asynchronous pm_request_resume() avoid blocking interrupt_work here?

[ ... ]

> @@ -935,7 +1003,7 @@ static int gmux_resume(struct device *dev)
>  	struct pnp_dev *pnp = to_pnp_dev(dev);
>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
>  
> -	gmux_enable_interrupts(gmux_data);
> +	gmux_enable_interrupts(gmux_data, 1);
>  	gmux_write_switch_state(gmux_data);
>  	if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
>  		gmux_set_discrete_state(gmux_data, gmux_data->power_state);

[Severity: High]
In gmux_suspend() right above gmux_resume(), gmux_disable_interrupts() is
called without cancelling gmux_data->interrupt_work:

static int gmux_suspend(struct device *dev)
{
	struct pnp_dev *pnp = to_pnp_dev(dev);
	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);

	gmux_disable_interrupts(gmux_data);
	return 0;
}

If gmux_data->interrupt_work is pending or running when gmux_suspend()
executes on an MMIO system, can gmux_notify_work() run concurrently with or
after gmux_suspend(), re-enabling interrupts via
gmux_enable_interrupts(gmux_data, 0) and calling
pm_runtime_resume(&gmux_data->discrete_pdev->dev) during system suspend?

[ ... ]

> @@ -1150,6 +1219,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  err_unmap:
>  	if (gmux_data->iomem_base)
>  		iounmap(gmux_data->iomem_base);
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);
>  err_release:
>  	if (gmux_data->config->resource_type == IORESOURCE_MEM)
>  		release_mem_region(gmux_data->iostart, gmux_data->iolen);

[Severity: High]
In the err_unmap error path of gmux_probe(), iounmap(gmux_data->iomem_base) is
called before cancel_work_sync(&gmux_data->interrupt_work). If
gmux_notify_work() was scheduled before gmux_probe() failed, won't
gmux_notify_work() access unmapped MMIO memory through gmux_data->iomem_base
before cancel_work_sync() waits for it to finish?

> @@ -1168,6 +1239,7 @@ static void gmux_remove(struct pnp_dev *pnp)
>  	vga_switcheroo_unregister_handler();
>  	pci_dev_put(gmux_data->discrete_pdev);
>  	gmux_disable_interrupts(gmux_data);
> +	cancel_work_sync(&gmux_data->interrupt_work);
>  	if (gmux_data->gpe >= 0) {
>  		acpi_disable_gpe(NULL, gmux_data->gpe);
>  		acpi_remove_notify_handler(gmux_data->dhandle,

[Severity: High]
In gmux_probe(), INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work) is
only called in the APPLE_GMUX_TYPE_MMIO case. On APPLE_GMUX_TYPE_PIO and
APPLE_GMUX_TYPE_INDEXED systems, won't calling
cancel_work_sync(&gmux_data->interrupt_work) unconditionally in gmux_remove()
pass an uninitialized work_struct (with work->func == NULL) to
__flush_work() and trigger a WARN_ON(!work->func) warning?

[Severity: Critical]
In gmux_remove(), is the teardown ordering around cancel_work_sync() safe
against a racing ACPI notification?

Currently gmux_remove() does:

	pci_dev_put(gmux_data->discrete_pdev);
	gmux_disable_interrupts(gmux_data);
	cancel_work_sync(&gmux_data->interrupt_work);
	if (gmux_data->gpe >= 0) {
		acpi_disable_gpe(NULL, gmux_data->gpe);
		acpi_remove_notify_handler(gmux_data->dhandle,
					   ACPI_DEVICE_NOTIFY,
					   &gmux_notify_handler);
	}
	...
	if (gmux_data->iomem_base) {
		iounmap(gmux_data->iomem_base);
		release_mem_region(gmux_data->iostart, gmux_data->iolen);
	} else
		release_region(gmux_data->iostart, gmux_data->iolen);
	apple_gmux_data = NULL;
	kfree(gmux_data);

First, pci_dev_put(gmux_data->discrete_pdev) and gmux_disable_interrupts() are
called before cancel_work_sync(), so an already-running gmux_notify_work() can
still dereference gmux_data->discrete_pdev in pm_runtime_resume() and call
gmux_enable_interrupts(gmux_data, 0) after interrupts were disabled.

Second, because acpi_disable_gpe() and acpi_remove_notify_handler() are called
after cancel_work_sync(&gmux_data->interrupt_work), any GPE or in-flight
execution of gmux_notify_handler() before acpi_remove_notify_handler()
completes will call schedule_work(&gmux_data->interrupt_work) again, causing
gmux_notify_work() to run concurrently with iounmap(gmux_data->iomem_base) and
kfree(gmux_data).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025040046.1602-1-atharvatiwarilinuxdev@gmail.com?part=4