Re: [PATCH v4 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Migrating to `devm_hwmon_device_register_with_info()` inverts the teardown order, causing a NULL pointer dereference race condition on module exit.
- [Medium] Using `devm` allocations in `module_init` incorrectly ties the hwmon device lifecycle to the driver bind/unbind lifecycle rather than the module lifecycle.
--

--- Patch [1]: [PATCH v4 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info ---
commit 64b6fb798f7a912d4278236bbe004e5659b03109
Author: Shih-Yuan Lee <fourdollars@debian.org>

hwmon: (applesmc) Convert to hwmon_device_register_with_info
    
Convert the driver to use the modern hwmon_device_register_with_info()
API. This migration moves the registration of standard sensor attributes
to the HWMON core.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 90a14a7f2c4c0..a93b1f91a35dd 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]
> @@ -1343,17 +1434,97 @@ static int __init applesmc_init(void)
>  	if (ret)
>  		goto out_smcreg;
>  
> -	ret = applesmc_create_nodes(fan_group, smcreg.fan_count);
> -	if (ret)
> +	/* allocate hwmon channel configs */
> +	temp_config = devm_kcalloc(&pdev->dev, smcreg.index_count + 1,
> +				   sizeof(*temp_config), GFP_KERNEL);

[Severity: Medium]
Does using devm_ allocations in a module initialization function cause issues
if the driver is unbound via sysfs?

Since this driver performs its setup in applesmc_init() rather than a standard
probe() function, the devm_ allocations are tied to the device-driver binding.

If an admin unbinds the driver via sysfs, devres_release_all() will free the
devm_kcalloc memory and the devm_hwmon device, but will leave non-devm managed
resources like the input device active. Additionally, without a probe()
function, rebinding won't recreate the hwmon device, leaving the driver broken
until a full module reload.

[ ... ]
> @@ -1398,12 +1566,9 @@ static int __init applesmc_init(void)
>  
>  static void __exit applesmc_exit(void)
>  {
> -	hwmon_device_unregister(hwmon_dev);

[Severity: High]
Can this lead to a NULL pointer dereference during module exit?

By replacing the manual hwmon_device_unregister() with a devm managed hwmon
device, the sysfs teardown order is delayed until platform_device_unregister()
is called at the very end of this function.

>  	applesmc_release_key_backlight();
>  	applesmc_release_light_sensor();
>  	applesmc_release_accelerometer();
> -	applesmc_destroy_nodes(temp_group);
> -	applesmc_destroy_nodes(fan_group);
>  	applesmc_destroy_nodes(info_group);
>  	applesmc_destroy_smcreg();
>  	platform_device_unregister(pdev);

However, applesmc_destroy_smcreg() is called right before the platform device
is unregistered, which frees and nullifies smcreg.index and smcreg.cache. This
creates a window where the hwmon sysfs files are still active and accessible,
but their backing structures are gone.

If a concurrent read to a sysfs file occurs during this window, it could
call into the new hwmon read callbacks and dereference the null pointer:

applesmc_hwmon_read() {
    ...
    case hwmon_temp:
        if (attr == hwmon_temp_input) {
            const char *key = smcreg.index[channel];
            s16 value;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711060634.5794-2-fourdollars@debian.org?part=1