Re: [PATCH v3 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect devm registration order causes a Use-After-Free vulnerability during device unbind or probe failure.
- [Medium] Unreferenced `completion` causes a guaranteed maximum timeout delay on every asynchronous sensor read.
--

commit 9e088a070299b309352aa9a88006e3927d0c50bd
Author: Tze Yee Ng <tze.yee.ng@altera.com>

hwmon: add Altera SoC FPGA hardware monitoring driver

Add a hardware monitor driver for Altera SoC FPGA devices using the Stratix 10
service layer. Sensor channels are selected based on the service layer
compatible string.

> diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwmon/socfpga-hwmon.c
[ ... ]
> +static int socfpga_hwmon_async_read(struct device *dev,
> +				    enum hwmon_sensor_types type,
> +				    struct stratix10_svc_client_msg *msg)
> +{
> +	struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
> +	struct stratix10_svc_cb_data data = {};
> +	struct completion completion;
> +	unsigned long wait_ret;
> +	void *handle = NULL;
> +	int status, index, ret;
> +
> +	init_completion(&completion);
> +
> +	for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) {
> +		status = stratix10_svc_async_send(priv->chan, msg, &handle,
> +						  NULL, NULL);
> +		if (status == 0)
> +			break;
> +		dev_warn(dev, "Failed to send async message: %d", status);
> +		msleep(HWMON_RETRY_SLEEP_MS);
> +	}
> +
> +	if (status && !handle) {
> +		dev_err(dev, "Failed to send async message after %u retries: %d\n",
> +			HWMON_ASYNC_MSG_RETRY, status);
> +		return status;
> +	}
> +
> +	/*
> +	 * The service layer never invokes the async callback, but wait
> +	 * before polling anyway: RSU uses the same pattern and the firmware
> +	 * needs time to complete the transaction after async_send().
> +	 */
> +	wait_ret = wait_for_completion_io_timeout(&completion, HWMON_TIMEOUT);

[Severity: Medium]
Will this cause a guaranteed timeout delay on every sensor read?

In socfpga_hwmon_async_read(), a local completion is initialized but is
never passed to any asynchronous function or callback (the function
stratix10_svc_async_send() receives NULL for the callback).

Because the completion can never be signaled, the execution will
unconditionally block in an uninterruptible sleep for the entire
HWMON_TIMEOUT duration on every single sensor read. Only after this maximum
timeout expires does the function proceed to poll for the response.

> +	if (wait_ret > 0)
> +		dev_dbg(dev, "Received async interrupt\n");
[ ... ]
> +static int socfpga_hwmon_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon",
> +							 priv,
> +							 &socfpga_hwmon_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		if (priv->async)
> +			stratix10_svc_remove_async_client(priv->chan);
> +		stratix10_svc_free_channel(priv->chan);
> +		return PTR_ERR(hwmon_dev);
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv);

[Severity: High]
Does this devres registration order create a use-after-free vulnerability
during device unbind?

In socfpga_hwmon_probe(), the hwmon device is registered via
devm_hwmon_device_register_with_info() before the cleanup action
socfpga_hwmon_release_svc is added via devm_add_action_or_reset().

Because devres unwinds actions in reverse order of addition (LIFO), upon
device unbind, socfpga_hwmon_release_svc will execute first, freeing the
service layer channel (priv->chan).

However, the hwmon device will still be active and its sysfs attributes
accessible to user space during this time. A concurrent sysfs read can
invoke socfpga_hwmon_read(), which will attempt to use the already-freed
priv->chan, resulting in a use-after-free.

> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +}

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