[PATCH v3 0/2] Drivers: hv: vmbus: fix dynamic ID races

[PATCH v3 0/2] Drivers: hv: vmbus: fix dynamic ID races

[Jiale Yao]

VMBus dynamic IDs have two races with their sysfs add and remove paths.

First, new_id_store() checks for an existing ID separately from inserting
the new entry, allowing concurrent writers to add duplicate GUIDs. Move the
check and insertion into one critical section.

Second, matching returns a pointer into the dynamic ID list after dropping
the lock. A concurrent remove_id write can free that entry before probe uses
it. Copy the ID while holding the lock and reject probe when a dynamic ID is
removed between matching and probing.

The series was found by static analysis. It has been regression tested in
Azure by Kameron Carr.

Changes in v3:
- Keep the original declaration order in vmbus_probe().
- Add Kameron Carr's Tested-by and Reviewed-by tags.

Changes in v2:
- Split the dynamic ID insertion TOCTOU into a preparatory patch.
- Reject probe when the matching ID disappears before the second lookup.
- Preserve the ID-less hvsock probe path.
- Use scoped locking and memcpy() for the dynamic ID copy.
- Document that the device ID is valid only during probe.
- Cite the corresponding PCI and USB dynamic ID lifetime fixes.
- Add Cc: stable@vger.kernel.org and use the VMBus subject prefix.

Link: https://lore.kernel.org/r/20261021143323.338146-1-yaojiale02@163.com
Link: https://lore.kernel.org/r/20261019085544.1834700-1-yaojiale02@163.com

Jiale Yao (2):
  Drivers: hv: vmbus: prevent duplicate dynamic IDs
  Drivers: hv: vmbus: copy dynamic IDs before dropping the lock

 drivers/hv/vmbus_drv.c | 52 +++++++++++++++++++++++++-----------------
 include/linux/hyperv.h |  1 +
 2 files changed, 32 insertions(+), 21 deletions(-)


base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.34.1

[PATCH v3 1/2] Drivers: hv: vmbus: prevent duplicate dynamic IDs

[Jiale Yao]

new_id_store() checks for an existing dynamic ID before calling
vmbus_add_dynid(), but the check and insertion use separate critical
sections. Two concurrent writers can therefore both observe that a GUID is
absent and add duplicate entries. Removing the GUID once then leaves the
duplicate active.

Move the duplicate check into vmbus_add_dynid() so it is performed under
the same lock as the list insertion.

Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
Cc: stable@vger.kernel.org
Tested-by: Kameron Carr <kameroncarr@linux.microsoft.com>
Reviewed-by: Kameron Carr <kameroncarr@linux.microsoft.com>
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/hv/vmbus_drv.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4692687cdbf3..1037117e2d81 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -737,7 +737,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *
  */
 static int vmbus_add_dynid(struct hv_driver *drv, guid_t *guid)
 {
-	struct vmbus_dynid *dynid;
+	struct vmbus_dynid *dynid, *existing;
 
 	dynid = kzalloc_obj(*dynid);
 	if (!dynid)
@@ -745,9 +745,16 @@ static int vmbus_add_dynid(struct hv_driver *drv, guid_t *guid)
 
 	dynid->id.guid = *guid;
 
-	spin_lock(&drv->dynids.lock);
-	list_add_tail(&dynid->node, &drv->dynids.list);
-	spin_unlock(&drv->dynids.lock);
+	scoped_guard(spinlock, &drv->dynids.lock) {
+		list_for_each_entry(existing, &drv->dynids.list, node) {
+			if (guid_equal(&existing->id.guid, guid)) {
+				kfree(dynid);
+				return -EEXIST;
+			}
+		}
+
+		list_add_tail(&dynid->node, &drv->dynids.list);
+	}
 
 	return driver_attach(&drv->driver);
 }
@@ -780,9 +787,6 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
 	if (retval)
 		return retval;
 
-	if (hv_vmbus_dynid_match(drv, &guid))
-		return -EEXIST;
-
 	retval = vmbus_add_dynid(drv, &guid);
 	if (retval)
 		return retval;
-- 
2.34.1

RE: [PATCH v3 1/2] Drivers: hv: vmbus: prevent duplicate dynamic IDs

[Michael Kelley]

From: Jiale Yao <yaojiale02@163.com> Sent: Monday, October 26, 2026 11:57 PM
> 
> new_id_store() checks for an existing dynamic ID before calling
> vmbus_add_dynid(), but the check and insertion use separate critical
> sections. Two concurrent writers can therefore both observe that a GUID is
> absent and add duplicate entries. Removing the GUID once then leaves the
> duplicate active.
> 
> Move the duplicate check into vmbus_add_dynid() so it is performed under
> the same lock as the list insertion.
> 
> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
> Cc: stable@vger.kernel.org
> Tested-by: Kameron Carr <kameroncarr@linux.microsoft.com>
> Reviewed-by: Kameron Carr <kameroncarr@linux.microsoft.com>
> Signed-off-by: Jiale Yao <yaojiale02@163.com>

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
>  drivers/hv/vmbus_drv.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4692687cdbf3..1037117e2d81 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -737,7 +737,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(const struct hv_driver *
>   */
>  static int vmbus_add_dynid(struct hv_driver *drv, guid_t *guid)
>  {
> -	struct vmbus_dynid *dynid;
> +	struct vmbus_dynid *dynid, *existing;
> 
>  	dynid = kzalloc_obj(*dynid);
>  	if (!dynid)
> @@ -745,9 +745,16 @@ static int vmbus_add_dynid(struct hv_driver *drv, guid_t *guid)
> 
>  	dynid->id.guid = *guid;
> 
> -	spin_lock(&drv->dynids.lock);
> -	list_add_tail(&dynid->node, &drv->dynids.list);
> -	spin_unlock(&drv->dynids.lock);
> +	scoped_guard(spinlock, &drv->dynids.lock) {
> +		list_for_each_entry(existing, &drv->dynids.list, node) {
> +			if (guid_equal(&existing->id.guid, guid)) {
> +				kfree(dynid);
> +				return -EEXIST;
> +			}
> +		}
> +
> +		list_add_tail(&dynid->node, &drv->dynids.list);
> +	}
> 
>  	return driver_attach(&drv->driver);
>  }
> @@ -780,9 +787,6 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  	if (retval)
>  		return retval;
> 
> -	if (hv_vmbus_dynid_match(drv, &guid))
> -		return -EEXIST;
> -
>  	retval = vmbus_add_dynid(drv, &guid);
>  	if (retval)
>  		return retval;
> --
> 2.34.1
> 

[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@vger.kernel.org
Tested-by: Kameron Carr <kameroncarr@linux.microsoft.com>
Reviewed-by: Kameron Carr <kameroncarr@linux.microsoft.com>
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 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

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

[Michael Kelley]

From: Jiale Yao <yaojiale02@163.com> Sent: Monday, October 26, 2026 11:57 PM
> 
> 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.

I had a little trouble mapping this paragraph to the code, but eventually
figured it out. For more clarity, I would have done this paragraph something like:

   Current code in vmbus_probe() assumes that hv_vmbus_get_id() will
   always succeed because vmbus_probe() isn't called unless there's an
   ID match. But matching and probing perform separate lookups, so
   removal can happen between them. Check the result of probe lookup,
   and reject the probe if it fails instead of allowing drivers to dereference
   NULL. But like in vmbus_match(), make an exception for hvsock because
   it does not use an ID table or inspect the ID.

Don't respin the patch set just for this change. Maybe Wei Liu can
tweak it when he applies the patch set.

> 
> This issue was found by static analysis of the dynamic ID lifetime.
> 
> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
> Cc: stable@vger.kernel.org
> Tested-by: Kameron Carr <kameroncarr@linux.microsoft.com>
> Reviewed-by: Kameron Carr <kameroncarr@linux.microsoft.com>
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  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)

This change to the function signature, and similarly for hv_vmbus_get_id(),
produces a rather unusual result. Upon success, the function returns the
new 2nd argument. It took me a little while to figure that out, just
because it's unexpected. A more normal function signature would be
to return a result code to indicate success or failure, with the new 2nd
argument containing the actual ID value that is wanted upon success.

I realize you are following the pattern established by PCI and USB
commits mentioned in your commit message. And there's real
value in consistency. So this is just commentary and not a request
for a code change. Leave it as you have written it for consistency,
but it is a bit weird. :-)

All that said,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

>  {
> -	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
>