Re: [PATCH v3 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] The async service layer is torn down before the child platform devices (HWMON and RSU) are unregistered, leading to a Use-After-Free and NULL pointer dereference if a concurrent async call is made.
--

commit 2c6317a7aef89428e616ea8354fab157b27ff35f
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. Register 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
introduce a race condition with module removal?

In stratix10_svc_drv_remove(), we tear down the async service layer before
the child devices are unregistered:

	stratix10_svc_async_exit(ctrl);
	of_platform_depopulate(ctrl->dev);
	if (svc->stratix10_svc_hwmon)
		platform_device_unregister(svc->stratix10_svc_hwmon);

If a sysfs read for the hwmon device (e.g., temp1_input) races with module
unload, could it proceed to stratix10_svc_async_send() because the device
is not yet unregistered?

If so, it looks like stratix10_svc_async_send() will dereference the achan
structure that was already freed:

	ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS,
			    GFP_KERNEL);

And later attempt a NULL pointer dereference, as actrl->invoke_fn was set
to NULL by stratix10_svc_async_exit():

	actrl->invoke_fn(actrl, &args, &res);

Should the child platform devices be unregistered before
stratix10_svc_async_exit() is called?

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

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