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 <[email protected]> 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/[email protected]?part=4
