[PATCH 0/2] scsi: mvsas: fix UAF races in mvs_free() teardown

Sangyun Kim posted 2 patches 1 month, 4 weeks ago
drivers/scsi/mvsas/mv_init.c | 13 +++++++++++--
drivers/scsi/mvsas/mv_sas.c  | 10 +++++++---
2 files changed, 18 insertions(+), 5 deletions(-)
[PATCH 0/2] scsi: mvsas: fix UAF races in mvs_free() teardown
Posted by Sangyun Kim 1 month, 4 weeks ago
Hi,

This series fixes two teardown-path use-after-free bugs in
drivers/scsi/mvsas that are reachable when mvs_pci_remove() runs
with pending delayed work on mvi->wq_list.

The fixes are split so each one addresses a single issue and bisects
cleanly.  Patch 2 depends on patch 1 (both touch mvs_free() and
patch 1 sets the ordering that patch 2 extends).

Patch 1 - "scsi: mvsas: drain delayed work before unmapping MMIO in
mvs_free()"

  mvs_free() calls MVS_CHIP_DISP->chip_iounmap(mvi) before the loop
  that cancels mwq->work_q.  If an mvs_work_queue() callback is
  scheduled or already running when chip_iounmap() executes, it
  dereferences the unmapped BAR via MVS_CHIP_DISP->read_phy_ctl() and
  takes a page fault.

  Move the cancel_delayed_work_sync() loop above chip_iounmap() (and
  above scsi_host_put()) so that no callback can touch MMIO or host
  state that has already been torn down.

Patch 2 - "scsi: mvsas: fix iterator use-after-free in mvs_free() wq
drain loop"

  mvs_free() walks mvi->wq_list with list_for_each_entry() while
  calling cancel_delayed_work_sync() on each node.  If the callback
  for the current node is already executing, mvs_work_queue() will
  list_del() and kfree() its own struct mvs_wq before the sync
  returns, and the iterator then dereferences the freed node's
  ->entry.next.  Converting to the _safe() variant is not enough
  because the saved "next" cursor can itself be freed by another
  callback while mvi->lock is dropped.

  Drain the list one entry at a time under mvi->lock: mvs_free()
  uses list_first_entry() + list_del_init() to detach the head
  before dropping the lock around cancel_delayed_work_sync() +
  kfree().  mvs_work_queue() checks list_empty(&mwq->entry) under
  mvi->lock and skips its own list_del_init() + kfree() when
  teardown has already claimed the entry, giving a single-owner
  rule for the struct mvs_wq free.

Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>

Sangyun Kim (2):
  scsi: mvsas: drain delayed work before unmapping MMIO in mvs_free()
  scsi: mvsas: fix iterator use-after-free in mvs_free() wq drain loop

 drivers/scsi/mvsas/mv_init.c | 13 +++++++++++--
 drivers/scsi/mvsas/mv_sas.c  | 10 +++++++---
 2 files changed, 18 insertions(+), 5 deletions(-)


base-commit: 772a896a56e0e3ef9424a025cec9176f9d8f4552
-- 
2.34.1