usb: typec: class: three lifetime/locking gaps around the mux/switch handles and mode_selection (drvdata UAF after provider unbind; no concurrent protocol for set/put; unlocked state_update vs delete)

[徐晗翔]

The typec class layer leaves the lifetime of its mux/switch machinery
to caller discipline that nothing enforces or documents, and three
concrete gaps fall out of that.  The reportable one is the provider
drvdata gap: a mux/switch handle holder keeps a device reference on
the provider's class device (class_find_device() takes get_device,
drivers/base/class.c:470), so after the PROVIDER driver unbinds,
typec_mux_unregister() only does device_unregister -- the mux_dev
shell (and its ->set callback pointer) survives as long as any owner
holds a handle, while the provider's devm private data is freed the
moment its .remove returns.  The next orientation/mode change then
calls mux_dev->set() (mux.c:393-412) into e.g.
qmp_combo_typec_mux_set() (phy-qcom-qmp-combo.c:4804) or
rk_udphy_typec_mux_set() (phy-rockchip-usbdp.c:1346), which fetch
devm drvdata of a freed object -- use-after-free read/write with
KASAN visibility; the same shape applies to the switch/retimer-side
providers with explicit removes (ptn36502, nb7vpq904m).  Two more
gaps in the same layer are contract-shaped and currently shielded
only by callers: (a) the aggregate typec_mux/typec_switch handles are
freshly allocated per get with no refcount/lock/RCU, typec_mux_set()
walks the callback list unlocked and typec_mux_put() kfrees on the
final put (the one in-tree violator of the resulting "drain before
put" discipline -- pmic_glink_altmode -- is reported separately); and
(b) mode_selection.c's exported state_update/delete pair has no
common lock or lifetime protocol at all (delete: partner->sel = NULL,
cancel_delayed_work_sync, mode_list_clean -- which does NOT take
sel->lock -- mutex_destroy, kfree; update: unlocked read of
partner->sel then mutex_lock + list access; today's only protection
is that every in-tree caller holds ucsi's con->lock, per the
enumeration below).  Fixes: a dead-flag + -ENODEV check in the set
path for (the drvdata gap), a documented drain+put contract (or a
class-layer primitive) for the handle protocol, and a lock/lifetime
contract (or in-delete locking) for mode_selection.

The detailed description:

* Gap 1 (reportable UAF): provider drvdata outlived by the handle

  Linux 7.3: the owner takes the handle via class_find_device()
  (mux.c:295) which get_device()s the mux_dev (class.c:470); the
  final typec_mux_put() (mux.c:376-391) drops it; the provider's
  unregister (mux.c:478-483) only device_unregister()s.  So:

      provider unbind: .remove -> typec_mux_unregister (shell kept)
                       .remove returns -> devres releases the
                       provider's private data (qmp/udphy/ptn)
      later mode/orientation change on the owner:
                       typec_mux_set(handle) -> mux_dev->set(...)
                       -> typec_mux_get_drvdata() -> FREED data

  Instances with this devm-drvdata pattern: qcom-qmp-combo (set
  :4804-4806, devm qmp, register :4924, unregister devm action
  :4894-4898/:4930), rockchip-usbdp (set :1346, register :1413-1415,
  unregister :1402-1406); ptn36502/nb7vpq904m have explicit removes
  but devm priv/regmap, and their own put does not cover an owner
  still holding the handle.  Nothing else closes this: module refs
  pin code; the parent-device reference pins the provider device
  struct, not its devm data; the unregister's device_del has no
  kernfs-drain semantics for this path (set is reached via the
  owner's private pointer, not sysfs).

  Fix: make the class layer mark the mux/switch dead at unregister
  (WRITE_ONCE flag) and have typec_mux_set()/typec_switch_set()
  return -ENODEV on it (READ_ONCE), or provide a drain primitive;
  a transitional provider-side option is to allocate the drvdata
  with the class device so the lifetimes coincide.

* Gap 2 (contract): aggregate handles have no concurrent protocol

  mux.c allocates a fresh aggregate handle per get (:82-129/:320-368)
  with no refcount/lock/RCU; typec_mux_set() (:393-412) walks the
  callback list unlocked; typec_mux_put() kfrees on the final put;
  and typec_unregister_altmode() even suggests the fragile order
  (put at class.c:712 before device_unregister at :713).  I audited
  all owners: UCSI/cros/tipd/gaokun serialize set-vs-put by their
  own construction (work drains, free_irq, notifier unregister --
  tabulated in my notes; the one in-tree violator,
  pmic_glink_altmode, has no cancel/flush at all and is reported
  separately).  The retimer class does it right (kref'd device,
  retimer.c:65-81) -- the mux/switch aggregate handles are the
  historical weak spot.  Fix: document the drain+put discipline in
  include/linux/usb/typec_mux.h (it currently says nothing about
  concurrency) or provide a class-layer drain+put primitive.

* Gap 3 (latent): mode_selection state_update vs delete

  mode_selection.c:176-197 (state_update: unlocked read of
  partner->sel at :179, then mutex_lock(&sel->lock) + list access)
  vs :271-283 (delete: partner->sel = NULL; cancel_delayed_work_sync
  (covers only that work); mode_list_clean() at :102-110 which does
  NOT take sel->lock and kfrees every node; mutex_destroy(); kfree
  (sel)).  Any overlap = UAF (locking a destroyed mutex; racing the
  list cleanup).  Today unreachable: the single update entry
  (ucsi_altmode_update_active, ucsi.c:354-384) is only called under
  con->lock (ucsi.c:1029/:1522/displayport.c:255 via the locked
  ucsi_con_mutex_lock/thunderbolt.c:109), and the only delete
  implementation (cros_ec) is likewise under con->lock
  (ucsi.c:1317-1318 from :2004/:1513/:1402/:2156) -- the header
  (typec_altmode.h:247-285) documents state_update as a generic
  altmode-driver entry point but states no locking precondition, so
  the first future caller that trusts the header reintroduces the
  UAF.  Fix: in-delete locking (mutex_lock/unlock(&sel->lock) before
  the cleanup) or a kref/RCU anchor, and document the contract.

  These three are reported together because they are the same layer
  and the same root style: safety outsourced to undocumented caller
  discipline.  The class layer's own retimer implementation shows
  the fix pattern.


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



Regards,
Hanxiang Xu
xuhx@xxxxxxxxxxx