Re: [PATCH 2/9] driver core: prevent device_add() during system shutdown

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in device_add() abort path during system shutdown leading to Use-After-Free.
--

--- Patch [2]: [PATCH 2/9] driver core: prevent device_add() during system shutdown ---
commit 17748fb46ccfdb0adadd634adbfe70a9fd601b9a
Author: Tarun Sahu <[email protected]>

driver core: prevent device_add() during system shutdown

This commit adds system state checks to device_add() to prevent new
devices from being added while the system is halting, powering off, or
restarting. It re-checks the state under devices_kset->list_lock to
avoid a TOCTOU race window with device_shutdown().

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
[ ... ]
> @@ -3699,6 +3720,18 @@ int device_add(struct device *dev)
>  		goto Error;
>  	}
>  
> +	/*
> +	 * Check system_state again under list_lock to prevent a TOCTOU race
> +	 * where device_shutdown() runs concurrently and misses this device.
> +	 */
> +	spin_lock(&devices_kset->list_lock);
> +	if (unlikely(system_is_shutting_down())) {
> +		spin_unlock(&devices_kset->list_lock);
> +		error = -ESHUTDOWN;
> +		goto ShutdownError;

[Severity: High]
Does this error path introduce a use-after-free race with device_shutdown()?

If device_add() successfully links the device to devices_kset->list via
kobject_add(), device_shutdown() could concurrently run and pull the
newly added device from the list:

device_shutdown() {
    ...
    spin_lock(&devices_kset->list_lock);
    ...
    dev = list_entry(devices_kset->list.prev, struct device, kobj.entry);
    ...
    list_del_init(&dev->kobj.entry);
    spin_unlock(&devices_kset->list_lock);
    ...
}

If device_add() then resumes, acquires the list lock, and sees
system_is_shutting_down(), it will jump to ShutdownError and return
-ESHUTDOWN.

The caller of device_add() will typically treat this as a failure and
free the device and its associated private data. Meanwhile,
device_shutdown() is already outside the lock and actively executing the
shutdown callbacks on that exact same device:

device_shutdown() {
    ...
    if (dev->bus && dev->bus->shutdown) {
        ...
        dev->bus->shutdown(dev);
    }
    ...
}

Can this lead to device_shutdown() accessing freed device memory?

> +	}
> +	spin_unlock(&devices_kset->list_lock);
> +
>  	/* notify platform of device entry */
>  	device_platform_notify(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2