power: supply: bq25890: pump-express work can call power_supply_changed after the psy is unregistered

[徐晗翔]

The bq25890 probe registers the power_supply with devm and registers
bq25890_non_devm_cleanup() as an earlier devm action.  Devres is LIFO,
so on unbind the psy is unregistered first and the pump-express work is
canceled only afterwards.  bq25890_remove() does not cancel the work.
The pump work can therefore run after power_supply_unregister() and call
power_supply_changed(bq->charger) (and set_property on the secondary
charger), dereferencing a psy that the core may already have freed.
This is a driver remove-order bug, not the core wrapper bug.

The detailed description:

* Test environment / affected version

  Linux 7.3.0 source tree (unmodified kernel.org 7.3), x86_64.  This is
  a source-level report.  I did not run it on bq25890 hardware: the
  KASAN build used for my core tests does not enable
  CONFIG_CHARGER_BQ25890 and has no I2C bq25890 device.  No reproducer
  is attached.  Line numbers are from 7.3.

* Mechanism

  Probe order:

      :1409  bq->secondary_chrg = power_supply_get_by_name(...)
      :1413  devm_add_action_or_reset(..., bq25890_release_secondary_chrg)
      :1477  INIT_DELAYED_WORK(&bq->pump_express_work, ...)
      :1522  devm_add_action_or_reset(..., bq25890_non_devm_cleanup)
      :1530  bq25890_power_supply_init()
        :1040  devm_power_supply_register(...)
      :1534  devm_request_threaded_irq(...)

  The comment at :1518-1520 says the cleanup action must be registered
  before bq25890_power_supply_init() so that it runs after devm
  unregisters the power_supply.  That is exactly the wrong order for a
  work that calls into the psy: the psy is gone before the work is
  canceled.

  bq25890_remove() (:1550-1563) only cancels the usb_phy work and resets
  the chip; it does not cancel pump_express_work.
  bq25890_non_devm_cleanup() (:1449-1460) calls
  cancel_delayed_work_sync(&bq->pump_express_work), but only after the
  psy unregister.

  bq25890_charger_external_power_changed() (:744-779) queues the work at
  :763-765.  The work (:1056-1116) sleeps/polls for up to several
  seconds (3 s poll + 1 s sleep per iteration), calls
  power_supply_set_property(secondary, ONLINE) at :1068/:1104, and
  power_supply_changed(bq->charger) at :1110.

  Once power_supply_unregister() has run and the last reference is gone,
  bq->charger is a dangling pointer; power_supply_changed() dereferences
  psy->changed_lock at its first line.  If a consumer still holds a
  reference, the psy survives but is already removed; the changed work
  then hits the core 5B problem.

* Concrete corruption / impact

  power_supply_changed() on the freed psy is a slab-use-after-free
  (spin_lock on the freed changed_lock, then schedule_work on the freed
  work item).  It can corrupt the workqueue or crash the kernel; if a
  consumer keeps the psy alive, the same call reaches the freed LED
  triggers described in the core 5B report on a 7.3 kernel without the
  9f7f7a859451 get/set guard.  I have not demonstrated
  privilege escalation, and I have not run this on hardware.

* Trigger model

  bq25890 with pump-express enabled and a DCP charger; unbind/remove
  while the pump work is queued or running.  The work deliberately
  sleeps, so the window is seconds, not a single instruction.  Removing
  the device or unbinding the driver requires root; a charger
  unplug/replug can also start the work.  No generic unprivileged
  userspace interface.

* Reproducer / results

  No reproducer attached.  This is a source-level report; I can test a
  proposed fix if bq25890 hardware is available.  The relevant call
  chain is fully visible in 7.3 at the line numbers above.



* Suggested direction

  Quiesce the work before unregistering the psy.  In bq25890_remove(),
  set a stop flag checked by bq25890_charger_external_power_changed(),
  cancel_delayed_work_sync(&bq->pump_express_work), then ensure the
  power_supply is unregistered after that (explicit unregister or a
  devres order that puts cleanup before unregister).  Reordering the
  cleanup action alone is not enough because external_power_changed()
  can requeue the work after the cancel.  Also clear or check
  bq->charger before use.  Report-first, no patch attached; happy to
  test a proposal.

  
Please keep me in Cc; I am not subscribed to the lists.


Regards,
Hanxiang Xu
xuhx@xxxxxxxxxxx