[PATCH] scsi: libsas: Fix use-after-free in sas_eh_handle_resets()

[PATCH] scsi: libsas: Fix use-after-free in sas_eh_handle_resets()

[Wentao Liang]

sas_eh_handle_resets() drops the reference it took on the domain device
with sas_put_device() and then accesses dev->state again while holding
ha->lock. The put may release the last reference, so the clear_bit()
can touch freed memory.

Clear the pending flag first and drop the reference last.

Fixes: 5db45bdc87ce ("[SCSI] libsas: enforce eh strategy handlers only in eh context")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/scsi/libsas/sas_scsi_host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index c83282733ec4..14ca19c93c32 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -680,10 +680,15 @@ static void sas_eh_handle_resets(struct Scsi_Host *shost)
 		if (test_and_clear_bit(SAS_DEV_RESET, &dev->state))
 			i->dft->lldd_I_T_nexus_reset(dev);
 
-		sas_put_device(dev);
 		spin_lock_irq(&ha->lock);
 		clear_bit(SAS_DEV_EH_PENDING, &dev->state);
 		ha->eh_active--;
+		spin_unlock_irq(&ha->lock);
+
+		/* dev->state is no longer used, this may be the last reference */
+		sas_put_device(dev);
+
+		spin_lock_irq(&ha->lock);
 	}
 	spin_unlock_irq(&ha->lock);
 }
-- 
2.34.1

Re: [PATCH] scsi: libsas: Fix use-after-free in sas_eh_handle_resets()

[John Garry]

On 9/17/26 15:39, Wentao Liang wrote:
> sas_eh_handle_resets() drops the reference it took on the domain device
> with sas_put_device() and then accesses dev->state again while holding
> ha->lock. The put may release the last reference, so the clear_bit()
> can touch freed memory.
> 
> Clear the pending flag first and drop the reference last.
> 
> Fixes: 5db45bdc87ce ("[SCSI] libsas: enforce eh strategy handlers only in eh context")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>   drivers/scsi/libsas/sas_scsi_host.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index c83282733ec4..14ca19c93c32 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -680,10 +680,15 @@ static void sas_eh_handle_resets(struct Scsi_Host *shost)
>   		if (test_and_clear_bit(SAS_DEV_RESET, &dev->state))
>   			i->dft->lldd_I_T_nexus_reset(dev);
>   
> -		sas_put_device(dev);
>   		spin_lock_irq(&ha->lock);
>   		clear_bit(SAS_DEV_EH_PENDING, &dev->state);
>   		ha->eh_active--;
> +		spin_unlock_irq(&ha->lock);
> +
> +		/* dev->state is no longer used, this may be the last reference */
> +		sas_put_device(dev);
> +
> +		spin_lock_irq(&ha->lock);
>   	}
>   	spin_unlock_irq(&ha->lock);
>   }


I think that the following is a better change:

----8<-----

--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -695,9 +695,9 @@ static void sas_eh_handle_resets(struct Scsi_Host 
*shost)
                 if (test_and_clear_bit(SAS_DEV_RESET, &dev->state))
                         i->dft->lldd_I_T_nexus_reset(dev);

+               clear_bit(SAS_DEV_EH_PENDING, &dev->state);
                 sas_put_device(dev);
                 spin_lock_irq(&ha->lock);
-               clear_bit(SAS_DEV_EH_PENDING, &dev->state);
                 ha->eh_active--;
         }
         spin_unlock_irq(&ha->lock);


---->8-----