[PATCH] scsi: mpt3sas: Fix use-after-free race in event log access

[PATCH] scsi: mpt3sas: Fix use-after-free race in event log access

[Chengfeng Ye]

A use-after-free race exists between ioctl operations accessing the
event log and device removal freeing it. The race occurs because
ioc->remove_host flag is set without synchronization, creating
a window where an ioctl can pass the removal check but still access
freed memory.

Race scenario:

  CPU0 (ioctl)                       CPU1 (device removal)
  ----------------                   ---------------------
  _ctl_ioctl_main()
    mutex_lock(&pci_access_mutex)
    if (!ioc->remove_host)
      [check passes]
                                     scsih_remove()
                                       ioc->remove_host = 1
                                       mpt3sas_ctl_release()
                                         kfree(ioc->event_log)

    _ctl_eventreport()
      copy_to_user(..., ioc->event_log, ...)  <- use-after-free
  mutex_unlock(&pci_access_mutex)

Fix by setting ioc->remove_host while holding pci_access_mutex. This
ensures the ioctl path either completes before removal starts, or sees
the flag and returns -EAGAIN.

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 7092d0debef3..3086e27a6293 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -11264,7 +11264,10 @@ static void scsih_remove(struct pci_dev *pdev)
 	if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc))
 		return;
 
+	/* Set remove_host flag under pci_access_mutex to synchronize with ioctl path */
+	mutex_lock(&ioc->pci_access_mutex);
 	ioc->remove_host = 1;
+	mutex_unlock(&ioc->pci_access_mutex);
 
 	if (!pci_device_is_present(pdev)) {
 		mpt3sas_base_pause_mq_polling(ioc);
-- 
2.25.1

Re: [PATCH] scsi: mpt3sas: Fix use-after-free race in event log access

[Markus Elfring]

…
> Fix by setting ioc->remove_host while holding pci_access_mutex. This
> ensures the ioctl path either completes before removal starts, or sees
> the flag and returns -EAGAIN.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?


…
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -11264,7 +11264,10 @@ static void scsih_remove(struct pci_dev *pdev)
>  	if (_scsih_get_shost_and_ioc(pdev, &shost, &ioc))
>  		return;
>  
> +	/* Set remove_host flag under pci_access_mutex to synchronize with ioctl path */
> +	mutex_lock(&ioc->pci_access_mutex);
>  	ioc->remove_host = 1;
> +	mutex_unlock(&ioc->pci_access_mutex);

Would it become feasible to apply a scoped_guard() call?
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/mutex.h#L253


>  
>  	if (!pci_device_is_present(pdev)) {
>  		mpt3sas_base_pause_mq_polling(ioc);


Regards,
Markus

Re: [PATCH] scsi: mpt3sas: Fix use-after-free race in event log access

[Chengfeng Ye]

> Would it become feasible to apply a scoped_guard() call?
> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

No problem, addressed the issues in the v2 patch.

Best regards,
Chengfeng

Re: scsi: mpt3sas: Fix use-after-free race in event log access

[Markus Elfring]

>> Would it become feasible to apply a scoped_guard() call?
>> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> 
> No problem, addressed the issues in the v2 patch.

Thanks for your positive feedback.
https://lore.kernel.org/linux-scsi/20260118070256.321184-1-dg573847474@gmail.com/

How do you think about to increase the application of scope-based resource management
occasionally also any more source code places?

Regards,
Markus

Re: scsi: mpt3sas: Fix use-after-free race in event log access

[Chengfeng Ye]

> How do you think about to increase the application of scope-based resource management
> occasionally also any more source code places?

Thanks for your suggestion on it. I think it is a very good practice
to get rid of problems like
lock-leak or double-locking, while keeping the code elegant.

I would certainly like to keep the practice while making my future
patch, and help adjust other
code to also use the scope-based locking when appropriate.

Best regards,
Chengfeng