Hi Kameron, At 2026-10-21 07:30:55, "Kameron Carr" <[email protected]> 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/[email protected]/ > > >> 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
