[PATCH v3 2/2] Drivers: hv: vmbus: copy dynamic IDs before dropping the lock

[Jiale Yao]

Dynamic IDs are freed by remove_id_store() while holding dynids.lock, so a
pointer into a dynamic ID entry is valid only while that lock is held.
hv_vmbus_dynid_match() currently returns such a pointer after unlocking. A
concurrent remove_id write can therefore free the entry before a driver
probe dereferences it.

Copy a matching dynamic ID under the lock and pass the caller-owned copy to
the probe callback. This follows the lifetime handling added for PCI by
commit 3ffc4c9690c3 ("PCI: Fix UAF when probe runs concurrent to dyn ID
removal") and for USB by commit ef8154d8b52d ("usb: fix UAF when probe runs
concurrent to dyn ID removal"). Document that the ID is valid only for the
duration of probe.

Because matching and probing perform separate lookups, removal can also
happen between them. Reject the probe if the second lookup no longer finds
an ID instead of passing NULL to drivers which dereference it. Keep the
hvsock exception because it does not use an ID table or inspect the ID.

This issue was found by static analysis of the dynamic ID lifetime.

Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
Cc: stable@xxxxxxxxxxxxxxx
Tested-by: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
---
 drivers/hv/vmbus_drv.c | 34 ++++++++++++++++++++--------------
 include/linux/hyperv.h |  1 +
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 1037117e2d81..1cacbc71266f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -661,21 +661,21 @@ hv_vmbus_dev_match(const struct hv_vmbus_device_id *id, const guid_t *guid)
 }
 
 static const struct hv_vmbus_device_id *
-hv_vmbus_dynid_match(struct hv_driver *drv, const guid_t *guid)
+hv_vmbus_dynid_match(struct hv_driver *drv, const guid_t *guid,
+		     struct hv_vmbus_device_id *id_copy)
 {
-	const struct hv_vmbus_device_id *id = NULL;
-	struct vmbus_dynid *dynid;
+	scoped_guard(spinlock, &drv->dynids.lock) {
+		struct vmbus_dynid *dynid;
 
-	spin_lock(&drv->dynids.lock);
-	list_for_each_entry(dynid, &drv->dynids.list, node) {
-		if (guid_equal(&dynid->id.guid, guid)) {
-			id = &dynid->id;
-			break;
+		list_for_each_entry(dynid, &drv->dynids.list, node) {
+			if (guid_equal(&dynid->id.guid, guid)) {
+				memcpy(id_copy, &dynid->id, sizeof(*id_copy));
+				return id_copy;
+			}
 		}
 	}
-	spin_unlock(&drv->dynids.lock);
 
-	return id;
+	return NULL;
 }
 
 static const struct hv_vmbus_device_id vmbus_device_null;
@@ -685,7 +685,8 @@ static const struct hv_vmbus_device_id vmbus_device_null;
  * If there is no match, return NULL.
  */
 static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *drv,
-							struct hv_device *dev)
+							struct hv_device *dev,
+							struct hv_vmbus_device_id *id_copy)
 {
 	const guid_t *guid = &dev->dev_type;
 	const struct hv_vmbus_device_id *id;
@@ -697,7 +698,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *
 		return NULL;
 
 	/* Look at the dynamic ids first, before the static ones */
-	id = hv_vmbus_dynid_match((struct hv_driver *)drv, guid);
+	id = hv_vmbus_dynid_match((struct hv_driver *)drv, guid, id_copy);
 	if (!id)
 		id = hv_vmbus_dev_match(drv->id_table, guid);
 
@@ -843,13 +844,14 @@ ATTRIBUTE_GROUPS(vmbus_drv);
 static int vmbus_match(struct device *device, const struct device_driver *driver)
 {
 	const struct hv_driver *drv = drv_to_hv_drv(driver);
+	struct hv_vmbus_device_id id_copy;
 	struct hv_device *hv_dev = device_to_hv_device(device);
 
 	/* The hv_sock driver handles all hv_sock offers. */
 	if (is_hvsock_channel(hv_dev->channel))
 		return drv->hvsock;
 
-	if (hv_vmbus_get_id(drv, hv_dev))
+	if (hv_vmbus_get_id(drv, hv_dev, &id_copy))
 		return 1;
 
 	return 0;
@@ -864,9 +866,13 @@ static int vmbus_probe(struct device *child_device)
 	struct hv_driver *drv =
 			drv_to_hv_drv(child_device->driver);
 	struct hv_device *dev = device_to_hv_device(child_device);
+	struct hv_vmbus_device_id dev_id_copy;
 	const struct hv_vmbus_device_id *dev_id;
 
-	dev_id = hv_vmbus_get_id(drv, dev);
+	dev_id = hv_vmbus_get_id(drv, dev, &dev_id_copy);
+	if (!dev_id && !is_hvsock_channel(dev->channel))
+		return -ENODEV;
+
 	if (drv->probe) {
 		ret = drv->probe(dev, dev_id);
 		if (ret != 0)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index e61f9a4cb7c3..1d5862725f24 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1264,6 +1264,7 @@ struct hv_driver {
 		struct list_head list;
 	} dynids;
 
+	/* The device ID pointer is only valid during the probe callback. */
 	int (*probe)(struct hv_device *, const struct hv_vmbus_device_id *);
 	void (*remove)(struct hv_device *dev);
 	void (*shutdown)(struct hv_device *);
-- 
2.34.1