Re: [PATCH v2 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Child devices (hwmon, RSU, and OF-populated devices) are unregistered after the async controller is torn down, leading to a Use-After-Free if a child driver accesses transactions while the parent driver is being removed.
--

commit 34dda5913b8cf85802a76938265cc19235241f5c
Author: Tze Yee Ng <tze.yee.ng@altera.com>

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

This commit adds asynchronous read support for hardware monitor temperature
and voltage commands in the Stratix 10 service layer. It also registers a
socfpga-hwmon platform device when hardware monitor support is enabled.

> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index c24ca5823078f..716b58f5a6170 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
[ ... ]
> @@ -2050,6 +2087,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
>  
>  	of_platform_depopulate(ctrl->dev);
>  
> +	if (svc->stratix10_svc_hwmon)
> +		platform_device_unregister(svc->stratix10_svc_hwmon);
> +

[Severity: High]
This is a pre-existing issue, but does unregistering the child devices here
lead to a use-after-free if a user is actively accessing them?

Looking at stratix10_svc_drv_remove() in drivers/firmware/stratix10-svc.c,
stratix10_svc_async_exit(ctrl) is called before the child devices are
unregistered:

static void stratix10_svc_drv_remove(struct platform_device *pdev)
{
	...
	stratix10_svc_async_exit(ctrl);

	of_platform_depopulate(ctrl->dev);

	if (svc->stratix10_svc_hwmon)
		platform_device_unregister(svc->stratix10_svc_hwmon);
	...
}

If a user is actively reading a hwmon sysfs attribute during driver
unbind, the child driver could be blocked waiting or polling in
socfpga_hwmon_async_read().

Because stratix10_svc_async_exit() forcefully frees all pending
transaction handlers, and the child endpoints are still active at this point,
the polling child thread could wake up and call stratix10_svc_async_poll()
or stratix10_svc_async_done() using the now-freed transaction handler,
triggering a use-after-free.

Could we unregister the child devices before tearing down the async
controller to avoid this?

>  	platform_device_unregister(svc->stratix10_svc_rsu);
>  
>  	for (i = 0; i < SVC_NUM_CHANNEL; i++) {

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