From 139f57343b3d6b26d9f01580123b2ba2d2150337 Mon Sep 17 00:00:00 2001 From: Laurence Oberman Date: Mon, 31 Aug 2026 07:59:17 -0400 Subject: [PATCH] scsi: mpi3mr: Fix use-after-free on tgt_dev->starget during target device refresh/update mpi3mr_refresh_tgtdevs() and mpi3mr_devinfochg_evt_bh() read tgt_dev->starget and immediately pass it to starget_for_each_device() without holding mrioc->tgtdev_lock. Every writer of this field -- mpi3mr_target_alloc(), mpi3mr_target_destroy(), mpi3mr_slave_destroy() and mpi3mr_sdev_init() -- correctly serializes access under tgtdev_lock, but these two read sites do not, which leaves a check-then-use window against the SCSI core's target teardown path (scsi_remove_target(), invoked e.g. via a concurrent host reset, sysfs "delete", or SCSI EH device offlining running independently of the fwevt workqueue). Sequence observed on production hardware, triggered on the mpi3mr0_fwevt_wrkr workqueue during a SAS topology change shortly after a controller reset: BUG: kernel NULL pointer dereference, address: 0000000000000058 RIP: scsi_is_host_device+0x7/0x20 Call Trace: starget_for_each_device+0x34/0x100 mpi3mr_refresh_tgtdevs+0x152/0x1d0 [mpi3mr] mpi3mr_fwevt_bh+0x514/0x6c0 [mpi3mr] mpi3mr_fwevt_worker+0x1a/0x50 [mpi3mr] process_one_work+0x194/0x380 worker_thread+0x2fe/0x410 mpi3mr_refresh_tgtdevs() reads tgt_dev->starget as non-NULL, but by the time starget_for_each_device() dereferences it, a concurrent mpi3mr_target_destroy() has already cleared tgt_dev->starget under tgtdev_lock and the SCSI/device core has freed the underlying scsi_target (and its embedded struct device). The stale pointer is then walked by dev_to_shost() -> scsi_is_host_device(), producing the NULL/garbage dereference above. Fix this by taking mrioc->tgtdev_lock around every read of tgt_dev->starget, matching the existing writer-side discipline. Since starget_for_each_device() and mpi3mr_update_sdev() can end up doing non-atomic work (e.g. queue_limits_commit_update()), the lock cannot be held across the whole call, so instead pin the target's device with get_device() while holding the lock, drop the lock, then run starget_for_each_device() against the pinned reference and put_device() afterwards. This closes the TOCTOU window instead of merely narrowing it. The same unlocked read-and-dereference pattern also exists earlier in mpi3mr_refresh_tgtdevs()'s first removal-scan loop (tgt_dev->starget->hostdata); fix it the same way by holding tgtdev_lock across that check, which is cheap since it only touches plain struct fields. Assisted-by: Claude:Sonnet5 [Claude Code] Signed-off-by: Laurence Oberman Acked-by: Chandrakanth Patil Link: https://patch.msgid.link/20260831120047.14690-1-loberman@redhat.com Signed-off-by: Martin K. Petersen (Oracle) --- drivers/scsi/mpi3mr/mpi3mr_os.c | 45 +++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c index f80a21ec161b..0f7380448718 100644 --- a/drivers/scsi/mpi3mr/mpi3mr_os.c +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c @@ -1094,10 +1094,13 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc) { struct mpi3mr_tgt_dev *tgtdev, *tgtdev_next; struct mpi3mr_stgt_priv_data *tgt_priv; + struct scsi_target *starget; + unsigned long flags; dprint_reset(mrioc, "refresh target devices: check for removals\n"); list_for_each_entry_safe(tgtdev, tgtdev_next, &mrioc->tgtdev_list, list) { + spin_lock_irqsave(&mrioc->tgtdev_lock, flags); if (((tgtdev->dev_handle == MPI3MR_INVALID_DEV_HANDLE) || tgtdev->is_hidden) && tgtdev->host_exposed && tgtdev->starget && @@ -1106,6 +1109,7 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc) tgt_priv->dev_removed = 1; atomic_set(&tgt_priv->block_io, 0); } + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); } list_for_each_entry_safe(tgtdev, tgtdev_next, &mrioc->tgtdev_list, @@ -1127,15 +1131,25 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc) tgtdev = NULL; list_for_each_entry(tgtdev, &mrioc->tgtdev_list, list) { if ((tgtdev->dev_handle != MPI3MR_INVALID_DEV_HANDLE) && - !tgtdev->is_hidden) { - if (!tgtdev->host_exposed) + !tgtdev->is_hidden) { + if (!tgtdev->host_exposed) { mpi3mr_report_tgtdev_to_host(mrioc, - tgtdev->perst_id); - else if (tgtdev->starget) - starget_for_each_device(tgtdev->starget, - (void *)tgtdev, mpi3mr_update_sdev); - } + tgtdev->perst_id); + continue; + } + spin_lock_irqsave(&mrioc->tgtdev_lock, flags); + starget = tgtdev->starget; + if (starget) + get_device(&starget->dev); + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); + if (starget) { + starget_for_each_device(starget, (void *)tgtdev, + mpi3mr_update_sdev); + put_device(&starget->dev); + } + } } + dprint_reset(mrioc, "refresh target devices: done\n"); } /** @@ -1515,6 +1529,8 @@ static void mpi3mr_devinfochg_evt_bh(struct mpi3mr_ioc *mrioc, struct mpi3_device_page0 *dev_pg0) { struct mpi3mr_tgt_dev *tgtdev = NULL; + struct scsi_target *starget; + unsigned long flags; u16 dev_handle = 0, perst_id = 0; perst_id = le16_to_cpu(dev_pg0->persistent_id); @@ -1535,9 +1551,18 @@ static void mpi3mr_devinfochg_evt_bh(struct mpi3mr_ioc *mrioc, mpi3mr_report_tgtdev_to_host(mrioc, perst_id); if (tgtdev->is_hidden && tgtdev->host_exposed) mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev); - if (!tgtdev->is_hidden && tgtdev->host_exposed && tgtdev->starget) - starget_for_each_device(tgtdev->starget, (void *)tgtdev, - mpi3mr_update_sdev); + if (!tgtdev->is_hidden && tgtdev->host_exposed) { + spin_lock_irqsave(&mrioc->tgtdev_lock, flags); + starget = tgtdev->starget; + if (starget) + get_device(&starget->dev); + spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); + if (starget) { + starget_for_each_device(starget, (void *)tgtdev, + mpi3mr_update_sdev); + put_device(&starget->dev); + } + } out: if (tgtdev) mpi3mr_tgtdev_put(tgtdev);