Re: [PATCH v6 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Hardware monitoring functionality is registered as a platform device rather than an auxiliary device, violating hwmon subsystem layering rules.

Pre-existing issues:
- [High] Asynchronous service layer APIs and resources are destroyed before client devices are unregistered, leading to a Use-After-Free and NULL pointer dereference race condition.
--

--- Patch [1]: [PATCH v6 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device ---
commit 552947f984c2b5770daa9717c679362b5561cfa3
Author: Tze Yee Ng <tze.yee.ng@altera.com>

firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device

Add asynchronous Stratix 10 service layer support for hardware monitor
temperature and voltage read commands in stratix10_svc_async_send() and
stratix10_svc_async_prepare_response().

Register a socfpga-hwmon platform device from the service layer driver
when hardware monitor support is enabled, similar to the RSU device.

> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index c24ca5823078f..fc38afed5b7fe 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
[ ... ]
> @@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_put_device;
>  
> +	if (IS_ENABLED(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)) {
> +		svc->stratix10_svc_hwmon =
> +			platform_device_alloc(SOCFPGA_HWMON, 0);
> +		if (!svc->stratix10_svc_hwmon) {
> +			dev_err(dev, "failed to allocate %s device\n",
> +				SOCFPGA_HWMON);
> +		} else {
> +			svc->stratix10_svc_hwmon->dev.parent = dev;
> +
> +			ret = platform_device_add(svc->stratix10_svc_hwmon);

[Severity: Medium]
Should the hardware monitoring functionality be implemented as an auxiliary
device driver instead of a platform device?

The hardware monitoring subsystem guidelines state that when hardware
monitoring is a secondary function of a chip, it should be implemented as an
auxiliary device and reside in drivers/hwmon/.

Does registering socfpga-hwmon directly as a platform device from this
firmware service driver bypass the architectural layering rules for hwmon?

[ ... ]
> @@ -2046,6 +2083,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
>  	struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
>  	struct stratix10_svc *svc = ctrl->svc;
>  
> +	if (svc->stratix10_svc_hwmon)
> +		platform_device_unregister(svc->stratix10_svc_hwmon);
> +
>  	stratix10_svc_async_exit(ctrl);
>  
>  	of_platform_depopulate(ctrl->dev);

[Severity: High]
This is a pre-existing issue, but can the destruction order in
stratix10_svc_drv_remove() lead to a NULL pointer dereference or a
use-after-free race condition?

When stratix10_svc_async_exit() is called, it destroys the async ID pool
and sets actrl->invoke_fn to NULL:

drivers/firmware/stratix10-svc.c:stratix10_svc_async_exit() {
    ...
    ida_destroy(&actrl->async_id_pool);
    actrl->invoke_fn = NULL;
    ...
}

However, the RSU client device is unregistered after this destruction step
via platform_device_unregister(svc->stratix10_svc_rsu).

If a user interacts with a client device's sysfs attribute while the module
is being unloaded, could the still-active client device invoke
stratix10_svc_async_send()?

If so, it would concurrently access freed asynchronous channels or attempt
to execute actrl->invoke_fn(actrl, &args, &res) after it has been locklessly
cleared, resulting in a kernel crash.

Would it be safer to unregister all child and client devices before the
resources they depend on are torn down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784516953.git.tze.yee.ng@altera.com?part=1