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 <[email protected]>
> ---
> 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
