Re: [PATCH v2] usb: typec: ucsi: acpi: fix use-after-free on driver removal

[Iván Rodriguez]

Hi,

Little ping on this patch. It has received RB tags from Wei Huang and
Heikki Krogerus, and I don't believe there are any outstanding
comments.

Please let me know if anything else is needed from my side.

Thanks,

Iván

On Mon, 7 Sep 2026 13:55:10 +0200, Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 03, 2026 at 08:21:21PM -0300, Iván Ezequiel Rodriguez wrote:
> > ucsi_acpi_remove() destroys the UCSI instance before removing the ACPI
> > notify handler:
> >
> > ucsi_unregister(ua->ucsi);
> > ucsi_destroy(ua->ucsi);
> >
> > acpi_remove_notify_handler(...);
> >
> > ucsi_acpi_notify() dereferences ua->ucsi, so a notify arriving after
> > ucsi_destroy() uses freed memory:
> >
> > CPU0 CPU1
> > ---- ----
> > ucsi_acpi_remove()
> > ucsi_unregister()
> > ucsi_destroy()
> > kfree(ucsi)
> > ucsi_acpi_notify()
> > ua->ucsi->ops->read_cci() <-- UAF
> >
> > Simply removing the handler before ucsi_unregister() is not correct
> > either. ucsi_unregister() drains work that needs the notify path to make
> > progress: ucsi_handle_connector_change() issues GET_CONNECTOR_STATUS and
> > ucsi_unregister_port() drains and destroys con->wq, and those commands
> > block in wait_for_completion_timeout() on ucsi->complete for up to
> > UCSI_TIMEOUT_MS. That completion is signalled only from
> > ucsi_notify_common(), i.e. from the notify handler. Tearing the handler
> > down first would leave cancel_work_sync() and destroy_workqueue()
> > waiting the full timeout for a completion that can no longer arrive.
> >
> > Moving the removal between ucsi_unregister() and ucsi_destroy() is not
> > sufficient on its own: at that point the connector array has already
> > been freed, so a late notify reaching ucsi_connector_change() would
> > queue work on a freed connector.
> >
> > Teardown therefore needs two properties at the same time: no new
> > connector changes once connectors start going away, but the notify path
> > still available for command and acknowledge completions until that work
> > has been drained. Whether the PPM actually produces those completions is
> > a firmware matter; what changes here is that the path able to deliver
> > them is no longer torn down first.
> >
> > Introduce a quiescing state, local to the ACPI backend, that provides
> > both. ucsi_acpi_remove() sets ua->quiescing under ua->notify_lock before
> > calling ucsi_unregister(); ucsi_acpi_notify() takes the same lock and,
> > when quiescing, reduces the CCI to the bits that ucsi_notify_common()
> > consumes for completions. ucsi_notify_common() looks at exactly
> > UCSI_CCI_BUSY, the connector number, UCSI_CCI_ACK_COMPLETE and
> > UCSI_CCI_COMMAND_COMPLETE; keeping the first and the last two preserves
> > the completion and bogus-data behaviour unchanged, while clearing the
> > connector number makes ucsi_connector_change() unreachable. The CCI that
> > the command path inspects is unaffected, because
> > ucsi_sync_control_common() re-reads it from the interface after the
> > completion.
> >
> > The resulting order is:
> >
> > mutex_lock(&ua->notify_lock);
> > ua->quiescing = true; -- no new connector work
> > mutex_unlock(&ua->notify_lock);
> >
> > ucsi_unregister(); -- drains work, notify path still
> > available for completions
> >
> > acpi_remove_notify_handler(); -- unlinks, then flushes
> > kacpi_notify_wq
> >
> > ucsi_destroy(); -- no notify can be in flight
> >
> > which gives the following happens-before chain:
> >
> > - A notify that acquires notify_lock before ucsi_acpi_remove() runs to
> > completion while remove() waits on the lock, so any schedule_work() it
> > performs happens before ucsi_unregister() starts cancelling.
> > - A notify that acquires notify_lock after remove() released it observes
> > quiescing == true, so it cannot reach ucsi_connector_change() and
> > cannot touch ucsi->connector.
> > - acpi_remove_notify_handler() unlinks the handler and then calls
> > acpi_os_wait_events_complete(), which flushes kacpi_notify_wq, so a
> > notify already dispatched on another CPU has returned before
> > ucsi_destroy() frees the instance.
> >
> > notify_lock is never held across ucsi_unregister() or
> > acpi_remove_notify_handler(); holding it there would deadlock against
> > the notify work those calls wait for. It is only ever taken as a leaf:
> > ucsi_notify_common() and ucsi_connector_change() take no locks, so it
> > cannot invert against ucsi->ppm_lock or con->lock, which the drained
> > work holds while waiting for the completion. The handler runs from
> > kacpi_notify_wq via acpi_os_execute(OSL_NOTIFY_HANDLER, ...), i.e. in
> > process context, so sleeping on the mutex is allowed.
> >
> > The probe error path already removes the handler before ucsi_destroy()
> > and is left unchanged.
> >
> > Fixes: f56de278e8ec ("usb: typec: ucsi: acpi: Move to the new API")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Fan Wu <fanwu01@xxxxxxxxxx>
> > Link: https://lore.kernel.org/all/20260718021142.3146566-1-fanwu01@xxxxxxxxxx/
> > Signed-off-by: Iván Ezequiel Rodriguez <ivanrwcm25@xxxxxxxxx>
>
> You could have used guard(mutex) in ucsi_acpi_notify(), but that's
> not a huge problem.
>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>
> > ---
> >
> > Notes:
> > Hi Heikki, Greg,
> >
> > v2 is a rewrite rather than an incremental fixup. v1 moved
> > acpi_remove_notify_handler() ahead of the teardown, and that ordering is
> > wrong for the reason Fan Wu had already documented in [1]: the work that
> > ucsi_unregister() drains can be waiting for a completion that only the
> > notify path delivers. This version keeps the handler installed across
> > ucsi_unregister() and adds an ACPI-local quiescing state instead.
> >
> > Wei Huang asked on v1 whether acpi_remove_notify_handler() waits for a
> > callback that already entered on another CPU. It does: for
> > ACPI_DEVICE_NOTIFY it calls acpi_os_wait_events_complete() after
> > unlinking the handler (drivers/acpi/acpica/evxface.c), that flushes
> > kacpi_notify_wq (drivers/acpi/osl.c), and device-notify dispatch runs on
> > that same workqueue through acpi_os_execute(OSL_NOTIFY_HANDLER, ...).
> > So the raw call is already the barrier, and acpi_dev_remove_notify_handler()
> > would only add a second flush. Chasing that question is what surfaced the
> > harder half of the problem, which is what this version is about.
> >
> > On [1]: that patch kept the handler installed across ucsi_unregister()
> > for exactly the right reason, and this version preserves that property.
> > What it did not cover is the window you described in that thread, where a
> > notify arriving after ucsi_unregister() has freed the connectors still
> > reaches ucsi_connector_change(). You also asked to keep the solution
> > inside ucsi_acpi.c rather than redesigning the core, which is what this
> > does.
> >
> > Changes since v1:
> > - Do not remove the notify handler before ucsi_unregister().
> > - Add the quiescing state, so connector changes stop while the notify
> > path stays available for completions.
> > - Drop the ucsi.c changes from v1 (ntfy = 0, connector = NULL, cap = 0).
> > I could not show they were needed for the other backends, and they do
> > not belong in the same patch as the ACPI lifetime fix.
> > - Correct the Fixes: tag. v1 pointed at 8243edf44152, which added the
> > driver; the current ordering came from f56de278e8ec.
> > - Credit Fan Wu, who reported the underlying use-after-free first.
> > - Use mutex_init() rather than devm_mutex_init(), which only appeared in
> > 4cd47222e435 (2024) and would be a needlessly modern dependency for a
> > fix tagged for stable from a 2019 commit.
> >
> > Testing
> >
> > I have no machine that exercises the UCSI ACPI path, so this was tested
> > with a software PPM backend that drives the real UCSI core
> > (ucsi_create/ucsi_register/ucsi_unregister/ucsi_destroy/
> > ucsi_notify_common) and reproduces the ACPI notify protocol, including
> > the deferral to a percpu workqueue. All three candidate teardown
> > orderings were run against it: the one from v1, the one from [1] and the
> > one in this patch. v7.3-rc2, KASAN + lockdep + PROVE_LOCKING +
> > DEBUG_MUTEXES, QEMU, oops=panic.
> >
> > Each case below parks a connector work in wait_for_completion_timeout()
> > before teardown starts, and asserts that precondition rather than
> > assuming it.
> >
> > teardown ordering result
> > ------------------------------------ ----------------------------
> > quiesce, unregister, unlink (this) 148 ms, clean, with a late
> > connector notify fired after
> > ucsi_unregister() returned
> > unlink, unregister (v1) 10595 ms stall
> > unregister, unlink (as in [1]), a KASAN slab-use-after-free in
> > connector notify landing in the queue_work_on(), then a GP
> > window fault in the kworker that
> > picked up the freed work
> > nothing in flight (this) 146 ms, clean
> >
> > Repeated with the teardown starting while ucsi_init_work() is still
> > running, so that cancel_delayed_work_sync(&ucsi->work) has to drain an
> > init command parked on the completion: this patch takes 2299 ms and
> > completes cleanly, of which 1500 ms is the injected command delay, while
> > the v1 ordering stalls for 10089 ms and the init gives up with
> > -ETIMEDOUT.
> >
> > Two deterministic checks of the properties the patch claims:
> >
> > - CCI mask. A single CCI carrying both connector 1 and COMMAND_COMPLETE
> > (0x80000002) is delivered while quiescing: the completion is signalled
> > and EVENT_PENDING stays clear, i.e. ucsi_connector_change() is not
> > reached. The same CCI with quiescing off sets EVENT_PENDING, so the
> > check is sensitive to the path it claims to block.
> >
> > - notify_lock as a barrier. A notify that has entered the handler is
> > held inside it for 1200 ms; the store of quiescing in the teardown
> > path blocks for 1215 ms behind it. This is what makes "a notify that
> > started before teardown finishes its schedule_work() before
> > ucsi_unregister() begins cancelling" an ordering guarantee rather
> > than a likelihood.
> >
> > Soak: 1000 teardown cycles with four threads hammering the notify path
> > concurrently with the quiesce sequence, repeated at 1, 2, 4 and 8 vCPUs,
> > so 4000 teardowns in total. No stall, no KASAN report and no lockdep
> > splat in any configuration. A separate KCSAN build ran 200 of those
> > cycles at 4 vCPUs with no data race reported in any UCSI path.
> >
> > What this does not cover: no real ACPI hardware, so the ACPICA drain
> > described above is established by reading evxface.c and osl.c rather
> > than by execution; and the LG gram quirk path is untouched and
> > unexercised.
> >
> > The harness is not part of this patch. I can post it separately if it
> > is useful.
> >
> > [1] https://lore.kernel.org/all/20260718021142.3146566-1-fanwu01@xxxxxxxxxx/
> >
> > drivers/usb/typec/ucsi/ucsi_acpi.c | 53 ++++++++++++++++++++++++++++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > index 18286d3e9cc5..61bba7625d17 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > @@ -24,6 +24,15 @@ struct ucsi_acpi {
> > bool check_bogus_event;
> > guid_t guid;
> > u64 cmd;
> > + /*
> > + * notify_lock serialises ucsi_acpi_notify() against the start of
> > + * teardown, so that @quiescing is observed by every notify that has
> > + * not yet run. It must not be held across ucsi_unregister(), whose
> > + * drained work may depend on the notify path, nor across
> > + * acpi_remove_notify_handler(), which flushes notify work.
> > + */
> > + struct mutex notify_lock;
> > + bool quiescing;
> > };
> >
> > static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> > @@ -179,11 +188,31 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
> > u32 cci;
> > int ret;
> >
> > + mutex_lock(&ua->notify_lock);
> > +
> > ret = ua->ucsi->ops->read_cci(ua->ucsi, &cci);
> > if (ret)
> > - return;
> > + goto out_unlock;
> > +
> > + /*
> > + * Once teardown has started the connectors are being unregistered and
> > + * freed, so a connector change must not be reported any more. Command
> > + * and acknowledge completions must still be able to reach the core:
> > + * ucsi_unregister() drains connector and partner work that can be
> > + * blocked in wait_for_completion_timeout() on ucsi->complete, and that
> > + * completion is only signalled from here. Keep exactly the bits that
> > + * ucsi_notify_common() needs for that, which drops the connector
> > + * number and with it the path to ucsi_connector_change(). The busy
> > + * indicator is kept so that bogus CCI data is still ignored.
> > + */
> > + if (ua->quiescing)
> > + cci &= UCSI_CCI_BUSY | UCSI_CCI_ACK_COMPLETE |
> > + UCSI_CCI_COMMAND_COMPLETE;
> >
> > ucsi_notify_common(ua->ucsi, cci);
> > +
> > +out_unlock:
> > + mutex_unlock(&ua->notify_lock);
> > }
> >
> > static int ucsi_acpi_probe(struct platform_device *pdev)
> > @@ -219,6 +248,8 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
> >
> > ua->dev = &pdev->dev;
> >
> > + mutex_init(&ua->notify_lock);
> > +
> > id = dmi_first_match(ucsi_acpi_quirks);
> > if (id)
> > ops = id->driver_data;
> > @@ -256,11 +287,29 @@ static void ucsi_acpi_remove(struct platform_device *pdev)
> > {
> > struct ucsi_acpi *ua = platform_get_drvdata(pdev);
> >
> > + /*
> > + * Stop reporting connector changes, but keep the notify handler
> > + * installed so that the work ucsi_unregister() drains can still be
> > + * reached by the command completions it may be waiting for. Any notify
> > + * that already passed this point runs to completion first, so no
> > + * connector work can be queued once ucsi_unregister() starts.
> > + */
> > + mutex_lock(&ua->notify_lock);
> > + ua->quiescing = true;
> > + mutex_unlock(&ua->notify_lock);
> > +
> > ucsi_unregister(ua->ucsi);
> > - ucsi_destroy(ua->ucsi);
> >
> > + /*
> > + * Now that no work is left to serve, drop the handler. This unlinks it
> > + * and then calls acpi_os_wait_events_complete(), which flushes
> > + * kacpi_notify_wq, so a notify running on another CPU has returned
> > + * before ucsi_destroy() frees the instance that it dereferences.
> > + */
> > acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
> > ucsi_acpi_notify);
> > +
> > + ucsi_destroy(ua->ucsi);
> > }
> >
> > static int ucsi_acpi_suspend(struct device *dev)
> > --
> > 2.43.0
>
> --
> heikki