[PATCH v2] scsi: libsas: Fix dev_list race conditions with proper locking

Chaohai Chen posted 1 patch 4 days, 18 hours ago
drivers/scsi/libsas/sas_discover.c | 14 ++++++++++++
drivers/scsi/libsas/sas_expander.c | 34 +++++++++++++++++++++++-------
drivers/scsi/libsas/sas_port.c     | 10 +++++++++
3 files changed, 50 insertions(+), 8 deletions(-)
[PATCH v2] scsi: libsas: Fix dev_list race conditions with proper locking
Posted by Chaohai Chen 4 days, 18 hours ago
Multiple functions in libsas were accessing port->dev_list without
proper locking, leading to potential race conditions that could cause:
- Use-after-free when devices are removed during list traversal
- List corruption from concurrent modifications
- System crashes from accessing freed memory

This patch adds proper dev_list_lock protection to the following functions:

1. sas_ex_level_discovery(): Added locking around list traversal with
   safe iteration and reference counting for devices. The lock is
   released before calling functions that may sleep
   (sas_ex_discover_devices).

2. sas_dev_present_in_domain(): Added locking for read-only list access
   to prevent reading inconsistent list state.

3. sas_suspend_devices(): Added locking around list traversal to prevent
   concurrent modifications during device suspension.

4. sas_unregister_domain_devices(): Added proper locking with reference
   counting. The lock is released before calling sas_unregister_dev()
   which may sleep, but device references are held to prevent premature
   removal.

5. sas_port_event_worker(): Added locking around list traversal with
   reference counting for devices accessed outside the lock.

All modifications follow the pattern of:
- Hold dev_list_lock during list traversal
- Use list_for_each_entry_safe where list may be modified
- Take device reference (kref_get) before releasing lock
- Release lock before calling functions that may sleep
- Release device reference (sas_put_device) after use

the conflict:
CPU 1: disco_q                          CPU 2: event_q
==============================          ==============================
sas_resume_devices()                    sas_porte_link_reset_err()

sas_resume_port()                       sas_deform_port(phy, true)

list_for_each_entry_safe(dev,           sas_unregister_domain_devices()
  &port->dev_list, ...)

NOP                                     free dev

visit dev->ex_dev(UAF)

Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
---
v1->v2:
- Change sas_dev_present_in_domain() to return a bool. (Damien Le Moal)
- Fix comment style. (Damien Le Moal)
- Make commit more detailed. (Damien Le Moal)
---
 drivers/scsi/libsas/sas_discover.c | 14 ++++++++++++
 drivers/scsi/libsas/sas_expander.c | 34 +++++++++++++++++++++++-------
 drivers/scsi/libsas/sas_port.c     | 10 +++++++++
 3 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index b07062db50b2..3c18fdfde8c2 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -245,8 +245,10 @@ static void sas_suspend_devices(struct work_struct *work)
 	 * suspension, we force the issue here to keep the reference
 	 * counts aligned
 	 */
+	spin_lock_irq(&port->dev_list_lock);
 	list_for_each_entry(dev, &port->dev_list, dev_list_node)
 		sas_notify_lldd_dev_gone(dev);
+	spin_unlock_irq(&port->dev_list_lock);
 
 	/* we are suspending, so we know events are disabled and
 	 * phy_list is not being mutated
@@ -410,11 +412,23 @@ void sas_unregister_domain_devices(struct asd_sas_port *port, bool gone)
 {
 	struct domain_device *dev, *n;
 
+	/* Lock while iterating to prevent concurrent modifications.
+	 * We need to unlock before calling sas_unregister_dev() as it
+	 * may sleep, but we hold a reference to prevent device removal.
+	 */
+	spin_lock_irq(&port->dev_list_lock);
 	list_for_each_entry_safe_reverse(dev, n, &port->dev_list, dev_list_node) {
 		if (gone)
 			set_bit(SAS_DEV_GONE, &dev->state);
+		kref_get(&dev->kref);
+		spin_unlock_irq(&port->dev_list_lock);
+
 		sas_unregister_dev(port, dev);
+		sas_put_device(dev);
+
+		spin_lock_irq(&port->dev_list_lock);
 	}
+	spin_unlock_irq(&port->dev_list_lock);
 
 	list_for_each_entry_safe(dev, n, &port->disco_list, disco_list_node)
 		sas_unregister_dev(port, dev);
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index d953225f6cc2..f91bd0601adc 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -639,18 +639,25 @@ static void sas_ex_disable_port(struct domain_device *dev, u8 *sas_addr)
 	}
 }
 
-static int sas_dev_present_in_domain(struct asd_sas_port *port,
+static bool sas_dev_present_in_domain(struct asd_sas_port *port,
 					    u8 *sas_addr)
 {
 	struct domain_device *dev;
+	bool found = false;
 
 	if (SAS_ADDR(port->sas_addr) == SAS_ADDR(sas_addr))
 		return 1;
+
+	spin_lock_irq(&port->dev_list_lock);
 	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
-		if (SAS_ADDR(dev->sas_addr) == SAS_ADDR(sas_addr))
-			return 1;
+		if (SAS_ADDR(dev->sas_addr) == SAS_ADDR(sas_addr)) {
+			found = true;
+			break;
+		}
 	}
-	return 0;
+	spin_unlock_irq(&port->dev_list_lock);
+
+	return found;
 }
 
 #define RPEL_REQ_SIZE	16
@@ -1579,20 +1586,31 @@ static int sas_discover_expander(struct domain_device *dev)
 static int sas_ex_level_discovery(struct asd_sas_port *port, const int level)
 {
 	int res = 0;
-	struct domain_device *dev;
+	struct domain_device *dev, *n;
 
-	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
+	spin_lock_irq(&port->dev_list_lock);
+	list_for_each_entry_safe(dev, n, &port->dev_list, dev_list_node) {
 		if (dev_is_expander(dev->dev_type)) {
 			struct sas_expander_device *ex =
 				rphy_to_expander_device(dev->rphy);
 
-			if (level == ex->level)
+			if (level == ex->level) {
+				kref_get(&dev->kref);
+				spin_unlock_irq(&port->dev_list_lock);
 				res = sas_ex_discover_devices(dev, -1);
-			else if (level > 0)
+				sas_put_device(dev);
+				spin_lock_irq(&port->dev_list_lock);
+			} else if (level > 0) {
+				kref_get(&port->port_dev->kref);
+				spin_unlock_irq(&port->dev_list_lock);
 				res = sas_ex_discover_devices(port->port_dev, -1);
+				sas_put_device(port->port_dev);
+				spin_lock_irq(&port->dev_list_lock);
+			}
 
 		}
 	}
+	spin_unlock_irq(&port->dev_list_lock);
 
 	return res;
 }
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index de7556070048..491c9f7104c6 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -44,13 +44,19 @@ static void sas_resume_port(struct asd_sas_phy *phy)
 	 * 1/ presume every device came back
 	 * 2/ force the next revalidation to check all expander phys
 	 */
+	spin_lock_irq(&port->dev_list_lock);
 	list_for_each_entry_safe(dev, n, &port->dev_list, dev_list_node) {
 		int i, rc;
 
+		kref_get(&dev->kref);
+		spin_unlock_irq(&port->dev_list_lock);
+
 		rc = sas_notify_lldd_dev_found(dev);
 		if (rc) {
 			sas_unregister_dev(port, dev);
 			sas_destruct_devices(port);
+			sas_put_device(dev);
+			spin_lock_irq(&port->dev_list_lock);
 			continue;
 		}
 
@@ -62,7 +68,11 @@ static void sas_resume_port(struct asd_sas_phy *phy)
 				phy->phy_change_count = -1;
 			}
 		}
+
+		sas_put_device(dev);
+		spin_lock_irq(&port->dev_list_lock);
 	}
+	spin_unlock_irq(&port->dev_list_lock);
 
 	sas_discover_event(port, DISCE_RESUME);
 }
-- 
2.43.7