Re: [PATCH v8 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 improperly registered as a platform device instead of an auxiliary device.

Pre-existing issues:
- [High] Pre-existing Use-After-Free race condition during module removal due to improper teardown ordering of child devices.
--

--- Patch [1]: [PATCH v8 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device ---
commit 5ba2e4fb155f074de568280aeede81883f87924e
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 from the service layer driver when hardware monitor support is enabled.

> 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);

[Severity: Medium]
According to the hardware monitoring subsystem guidelines, if the main
functionality of a chip is not hardware monitoring, its secondary hardware
monitoring functionality should be implemented as an auxiliary device driver.

Since the primary purpose of this module is the firmware service layer,
does registering the hardware monitoring functionality as a platform device
violate the subsystem API layering? Could this be implemented via the
auxiliary bus instead of using platform_device_alloc()?

> +		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);
> +			if (ret) {
> +				dev_err(dev, "failed to add %s device: %d\n",
> +					SOCFPGA_HWMON, ret);
> +				platform_device_put(svc->stratix10_svc_hwmon);
> +				svc->stratix10_svc_hwmon = NULL;
> +			}
> +		}
> +	}
> +
>  	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);

[ ... ]

> @@ -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 does this teardown order create a
use-after-free race condition during module removal?

The stratix10_svc_drv_remove() function calls stratix10_svc_async_exit()
before unregistering the remaining OF child devices and the RSU child device.
The async exit function destroys the async structures and frees the
asynchronous channels and handlers.

If userspace concurrently accesses sysfs attributes for the child devices
while the module unloads, could it invoke stratix10_svc_async_send()
or stratix10_svc_async_poll() after the async channels have been freed?

For example, stratix10_svc_async_send() performs lockless reads of achan:

stratix10_svc_async_send() {
	...
	achan = chan->async_chan;
	ctrl = chan->ctrl;
	actrl = &ctrl->actrl;

	if (!actrl->initialized) {
	...
}

Would this lead to memory corruption if the structures are already deallocated?
Should the child devices be fully unregistered before calling
stratix10_svc_async_exit() to ensure no concurrent accesses are possible?

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