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

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

[Jiale Yao]

remove_id_store() can free a matching dynamic ID while VMBus is
preparing to invoke a driver probe. Copy the ID while holding the
dynamic ID lock so probe never receives a pointer into a freed list
entry.

Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/hv/vmbus_drv.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4692687cdbf3..96ad0dfaa50b 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -661,21 +661,22 @@ 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;
 
 	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;
+			*id_copy = dynid->id;
+			spin_unlock(&drv->dynids.lock);
+			return id_copy;
 		}
 	}
 	spin_unlock(&drv->dynids.lock);
 
-	return id;
+	return NULL;
 }
 
 static const struct hv_vmbus_device_id vmbus_device_null;
@@ -685,7 +686,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 +699,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);
 
@@ -773,6 +775,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
 			    size_t count)
 {
 	struct hv_driver *drv = drv_to_hv_drv(driver);
+	struct hv_vmbus_device_id id_copy;
 	guid_t guid;
 	ssize_t retval;
 
@@ -780,7 +783,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
 	if (retval)
 		return retval;
 
-	if (hv_vmbus_dynid_match(drv, &guid))
+	if (hv_vmbus_dynid_match(drv, &guid, &id_copy))
 		return -EEXIST;
 
 	retval = vmbus_add_dynid(drv, &guid);
@@ -840,12 +843,13 @@ static int vmbus_match(struct device *device, const struct device_driver *driver
 {
 	const struct hv_driver *drv = drv_to_hv_drv(driver);
 	struct hv_device *hv_dev = device_to_hv_device(device);
+	struct hv_vmbus_device_id id_copy;
 
 	/* 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;
@@ -860,9 +864,10 @@ 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 (drv->probe) {
 		ret = drv->probe(dev, dev_id);
 		if (ret != 0)
-- 
2.34.1

Re: [PATCH] hv/vmbus: copy dynamic IDs before dropping the lock

[Greg Kroah-Hartman]

On Mon, Oct 19, 2026 at 04:55:44PM +0800, Jiale Yao wrote:
> remove_id_store() can free a matching dynamic ID while VMBus is
> preparing to invoke a driver probe.

And how can that be triggered?

> Copy the ID while holding the dynamic ID lock so probe never receives
> a pointer into a freed list entry.

Why is this driver allowed to handle dynamic ids at all?  Is it really
required?  That will taint the kernel so no real workflow requires it,
right?

> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")

No cc: stable?

> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/hv/vmbus_drv.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4692687cdbf3..96ad0dfaa50b 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -661,21 +661,22 @@ 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;
>  
>  	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;
> +			*id_copy = dynid->id;

Shouldn't this be a memcpy() as that's what you are really doing?

> +			spin_unlock(&drv->dynids.lock);

guard() usage instead?

> +			return id_copy;
>  		}
>  	}
>  	spin_unlock(&drv->dynids.lock);
>  
> -	return id;
> +	return NULL;
>  }
>  
>  static const struct hv_vmbus_device_id vmbus_device_null;
> @@ -685,7 +686,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 +699,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);
>  
> @@ -773,6 +775,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  			    size_t count)
>  {
>  	struct hv_driver *drv = drv_to_hv_drv(driver);
> +	struct hv_vmbus_device_id id_copy;

On the stack?

Ok, I guess this would work, but it feels clunky, why doesn't all other
busses have to do this too?

thanks,

greg k-h

Re: [PATCH] hv/vmbus: copy dynamic IDs before dropping the lock

[Kameron Carr]

On 10/19/2026 1:55 AM, Jiale Yao wrote:
> remove_id_store() can free a matching dynamic ID while VMBus is
> preparing to invoke a driver probe. Copy the ID while holding the
> dynamic ID lock so probe never receives a pointer into a freed list
> entry.

Is this the same fix as:

  3ffc4c9690c3 ("PCI: Fix UAF when probe runs concurrent to dyn ID
  removal")
  ef8154d8b52d ("usb: fix UAF when probe runs concurrent to
  dyn ID removal")

If so, consider citing them in the commit message. Also, consider taking
inspiration from their implementation.

Nit: this subsystem typically uses "Drivers: hv: vmbus:" for the subject
prefix.


> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/hv/vmbus_drv.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4692687cdbf3..96ad0dfaa50b 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -661,21 +661,22 @@ 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;
>  
>  	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;
> +			*id_copy = dynid->id;
> +			spin_unlock(&drv->dynids.lock);
> +			return id_copy;
>  		}
>  	}
>  	spin_unlock(&drv->dynids.lock);
>  
> -	return id;
> +	return NULL;
>  }
>  
>  static const struct hv_vmbus_device_id vmbus_device_null;
> @@ -685,7 +686,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 +699,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);
>  
> @@ -773,6 +775,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  			    size_t count)
>  {
>  	struct hv_driver *drv = drv_to_hv_drv(driver);
> +	struct hv_vmbus_device_id id_copy;
>  	guid_t guid;
>  	ssize_t retval;
>  
> @@ -780,7 +783,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  	if (retval)
>  		return retval;
>  
> -	if (hv_vmbus_dynid_match(drv, &guid))
> +	if (hv_vmbus_dynid_match(drv, &guid, &id_copy))
>  		return -EEXIST;
>  
>  	retval = vmbus_add_dynid(drv, &guid);

This check and the list_add_tail() in vmbus_add_dynid() run in separate
critical sections, so two concurrent writers can both add the same GUID.
Consider a preparatory patch that moves the check into vmbus_add_dynid()
under the same dynids.lock; see 04fde70f782b ("PCI: Fix dyn_id add
TOCTOU"). new_id_store() then no longer needs hv_vmbus_dynid_match() or
id_copy at all.

> @@ -840,12 +843,13 @@ static int vmbus_match(struct device *device, const struct device_driver *driver
>  {
>  	const struct hv_driver *drv = drv_to_hv_drv(driver);
>  	struct hv_device *hv_dev = device_to_hv_device(device);
> +	struct hv_vmbus_device_id id_copy;
>  
>  	/* 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;
> @@ -860,9 +864,10 @@ 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);

Consider documenting in include/linux/hyperv.h that the
hv_vmbus_device_id is only valid during probe so future drivers don't
save the pointer (see what PCI and USB fixes did in pci.h and usb.h).

>  	if (drv->probe) {
>  		ret = drv->probe(dev, dev_id);
>  		if (ret != 0)

Please review the Sashiko comment.

Regards,
Kameron

Re:Re: [PATCH] hv/vmbus: copy dynamic IDs before dropping the lock

[jiale yao]

Hi Kameron,

At 2026-10-21 07:30:55, "Kameron Carr" <kameroncarr@linux.microsoft.com> wrote:
>On 10/19/2026 1:55 AM, Jiale Yao wrote:
>> remove_id_store() can free a matching dynamic ID while VMBus is
>> preparing to invoke a driver probe. Copy the ID while holding the
>> dynamic ID lock so probe never receives a pointer into a freed list
>> entry.
>
>Is this the same fix as:
>
>  3ffc4c9690c3 ("PCI: Fix UAF when probe runs concurrent to dyn ID
>  removal")
>  ef8154d8b52d ("usb: fix UAF when probe runs concurrent to
>  dyn ID removal")
>
>If so, consider citing them in the commit message. Also, consider taking
>inspiration from their implementation.

Yes!

>
>Nit: this subsystem typically uses "Drivers: hv: vmbus:" for the subject
>prefix.
Thanks for your review, how about this one
https://lore.kernel.org/all/20261021143323.338146-1-yaojiale02@163.com/

>
>
>> Fixes: fc76936d3ea5 ("vmbus: add support for dynamic device id's")
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/hv/vmbus_drv.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 4692687cdbf3..96ad0dfaa50b 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -661,21 +661,22 @@ 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;
>>  
>>  	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;
>> +			*id_copy = dynid->id;
>> +			spin_unlock(&drv->dynids.lock);
>> +			return id_copy;
>>  		}
>>  	}
>>  	spin_unlock(&drv->dynids.lock);
>>  
>> -	return id;
>> +	return NULL;
>>  }
>>  
>>  static const struct hv_vmbus_device_id vmbus_device_null;
>> @@ -685,7 +686,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 +699,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);
>>  
>> @@ -773,6 +775,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>>  			    size_t count)
>>  {
>>  	struct hv_driver *drv = drv_to_hv_drv(driver);
>> +	struct hv_vmbus_device_id id_copy;
>>  	guid_t guid;
>>  	ssize_t retval;
>>  
>> @@ -780,7 +783,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>>  	if (retval)
>>  		return retval;
>>  
>> -	if (hv_vmbus_dynid_match(drv, &guid))
>> +	if (hv_vmbus_dynid_match(drv, &guid, &id_copy))
>>  		return -EEXIST;
>>  
>>  	retval = vmbus_add_dynid(drv, &guid);
>
>This check and the list_add_tail() in vmbus_add_dynid() run in separate
>critical sections, so two concurrent writers can both add the same GUID.
>Consider a preparatory patch that moves the check into vmbus_add_dynid()
>under the same dynids.lock; see 04fde70f782b ("PCI: Fix dyn_id add
>TOCTOU"). new_id_store() then no longer needs hv_vmbus_dynid_match() or
>id_copy at all.
>
>> @@ -840,12 +843,13 @@ static int vmbus_match(struct device *device, const struct device_driver *driver
>>  {
>>  	const struct hv_driver *drv = drv_to_hv_drv(driver);
>>  	struct hv_device *hv_dev = device_to_hv_device(device);
>> +	struct hv_vmbus_device_id id_copy;
>>  
>>  	/* 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;
>> @@ -860,9 +864,10 @@ 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);
>
>Consider documenting in include/linux/hyperv.h that the
>hv_vmbus_device_id is only valid during probe so future drivers don't
>save the pointer (see what PCI and USB fixes did in pci.h and usb.h).
>
>>  	if (drv->probe) {
>>  		ret = drv->probe(dev, dev_id);
>>  		if (ret != 0)
>
>Please review the Sashiko comment.
>
>Regards,
>Kameron