Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] A time-of-check to time-of-use (TOCTOU) race condition in `vmbus_probe()` allows `drv->probe` to be called with a NULL `dev_id`, leading to a NULL pointer dereference in drivers like `storvsc_probe()`. -- commit 76d8c5c515ad429741f331c31ec380bf773956cb Author: Runyu Xiao <[email protected]> vmbus: use generic driver_override infrastructure This commit switches the vmbus subsystem to use the generic driver core driver_override infrastructure rather than bus-private storage. It removes the private driver_override storage and updates the match path to use device_match_driver_override() for locked reads, preventing the match logic from inspecting freed memory during concurrent updates. Link: https://lore.kernel.org/driver-core/[email protected]/ > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index d28ff45d4cfdd..a81e2b0976367 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c [ ... ] > @@ -708,9 +679,11 @@ static const struct hv_vmbus_device_id > *hv_vmbus_get_id(const struct hv_driver * > { > const guid_t *guid = &dev->dev_type; > const struct hv_vmbus_device_id *id; > + int ret; > > /* When driver_override is set, only bind to the matching driver */ > - if (dev->driver_override && strcmp(dev->driver_override, drv->name)) > + ret = device_match_driver_override(&dev->device, &drv->driver); > + if (ret == 0) > return NULL; [Severity: High] This is a pre-existing issue, but does this behavior still leave a time-of-check to time-of-use race condition open when called later during probing? The driver core __driver_attach() calls driver_match_device() locklessly, which delegates to vmbus_match() and eventually hv_vmbus_get_id(). If a matching driver name is written to the driver_override sysfs attribute (or a matching dynamic ID is present), it returns a match. Before __driver_attach() acquires the device lock to call driver_probe_device(), a concurrent sysfs write can clear the driver_override attribute or remove the dynamic ID. Inside vmbus_probe(), which executes with the device lock held, hv_vmbus_get_id() is called again. Because the state was changed concurrently, it can evaluate to NULL: drivers/hv/vmbus_drv.c:vmbus_probe() { ... dev_id = hv_vmbus_get_id(drv, dev); if (drv->probe) { ret = drv->probe(dev, dev_id); ... } Since vmbus_probe() lacks a NULL check for dev_id, it directly calls drv->probe(dev, dev_id) with dev_id equal to NULL. Sub-drivers like storvsc_probe() then unconditionally dereference dev_id (for example, checking dev_id->driver_data), which results in a kernel panic. Could a NULL check for dev_id be added in vmbus_probe() to fully prevent this race? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
