Re: [PATCH 4/5] hwmon: add Stratix 10 SoC FPGA hardware monitor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Use-After-Free due to improper devm lifecycle management during module removal.
- [High] Negative temperatures are miscalculated as massive positive values due to an implicit unsigned arithmetic promotion.
- [High] Synchronous read timeouts abandon requests in the FIFO, permanently desynchronizing responses.
- [Medium] Synchronous reads silently mask hardware failures and return stale sensor data instead of errors.
- [Low] Improper initialization of a stack-allocated completion variable.
- [Low] Incorrect error code returned for hardware failure.
--

commit 6b98d9d7f6f87d18500b9518e6aa77d54c53dc50
Author: Tze Yee Ng <tze.yee.ng@altera.com>
Subject: hwmon: add Stratix 10 SoC FPGA hardware monitor driver

This commit adds a hardware monitoring driver for Altera Stratix 10 SoC FPGA
devices. It supports reading temperature and voltage sensors via the Stratix
10 service layer. The driver defaults to asynchronous interfaces but falls
back to synchronous reads if necessary.

> diff --git a/drivers/hwmon/stratix10-hwmon.c b/drivers/hwmon/stratix10-hwmon.c
> new file mode 100644
> index 0000000000000..7ed1116e57b87
> --- /dev/null
> +++ b/drivers/hwmon/stratix10-hwmon.c
> @@ -0,0 +1,575 @@

[ ... ]

> +#define STRATIX10_HWMON_ATTR_VISIBLE	0444
> +/* Temperature from SDM is signed Q8.8 millidegrees Celsius (8 fractional bits). */
> +#define STRATIX10_HWMON_TEMP_FRAC_BITS	8
> +#define STRATIX10_HWMON_TEMP_FRAC_DIV	BIT(STRATIX10_HWMON_TEMP_FRAC_BITS)
> +/* Voltage from SDM is unsigned Q16 (millivolts, 16 fractional bits). */

[ ... ]

> +static int stratix10_hwmon_parse_temp(long *val, u32 temperature)
> +{
> +	switch (temperature) {
> +	case ETEMP_INACTIVE:
> +	case ETEMP_NOT_PRESENT:
> +	case ETEMP_CORRUPT:
> +	case ETEMP_NOT_INITIALIZED:
> +		return -EOPNOTSUPP;
> +	case ETEMP_TIMEOUT:
> +	case ETEMP_BUSY:
> +	case ETEMP_TOO_OLD:
> +		return -EAGAIN;
> +	default:
> +		/* Convert Q8.8 millidegrees Celsius to millidegrees for hwmon. */
> +		*val = (long)(s32)temperature / STRATIX10_HWMON_TEMP_FRAC_DIV;

[Severity: High]
Will negative temperatures be miscalculated as massive positive values here?

The macro STRATIX10_HWMON_TEMP_FRAC_DIV is defined using BIT(), which expands
to an unsigned long literal.

When dividing a signed long by an unsigned long, C promotion rules convert
the signed operand to unsigned. This means a negative temperature is converted
into a huge positive integer prior to division, entirely corrupting the
resulting sensor reading.

[ ... ]

> +static int stratix10_hwmon_async_read(struct device *dev,
> +				      enum hwmon_sensor_types type,
> +				     struct stratix10_svc_client_msg *msg)
> +{
> +	struct stratix10_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);

[Severity: Low]
Should this use init_completion_onstack() instead?

Initializing a stack-allocated completion with init_completion() can trigger
false positive circular locking dependency warnings from lockdep.

[ ... ]

> +	if (ret) {
> +		dev_err(dev, "Failed to get async response\n");
> +		goto done;
> +	}
> +
> +	if (data.status) {
> +		dev_err(dev, "%s returned 0x%x from SDM\n", __func__,
> +			data.status);
> +		ret = -EFAULT;

[Severity: Low]
Is -EFAULT the correct error code here?

In the kernel, -EFAULT typically denotes a bad user-space memory address.
Returning it for a hardware or firmware failure incorrectly implies that
user-space passed a bad pointer to a syscall. A more standard error code
such as -EIO or -ENODATA might be more appropriate.

> +		goto done;
> +	}

[ ... ]

> +static int stratix10_hwmon_sync_read(struct device *dev,
> +				     enum hwmon_sensor_types type,
> +				    struct stratix10_svc_client_msg *msg)
> +{
> +	struct stratix10_hwmon_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	reinit_completion(&priv->completion);
> +
> +	if (type == hwmon_temp)
> +		priv->client.receive_cb = stratix10_hwmon_readtemp_cb;
> +	else
> +		priv->client.receive_cb = stratix10_hwmon_readvolt_cb;
> +
> +	ret = stratix10_svc_send(priv->chan, msg);
> +	if (ret < 0)
> +		goto status_done;
> +
> +	ret = wait_for_completion_interruptible_timeout(&priv->completion,
> +							HWMON_TIMEOUT);
> +	if (!ret) {
> +		dev_err(priv->client.dev, "timeout waiting for SMC call\n");
> +		ret = -ETIMEDOUT;
> +		goto status_done;
> +	}

[Severity: High]
Does a timeout here permanently desynchronize responses?

The timeout path abandons the unread request in chan->svc_fifo without
flushing it.

When a subsequent sysfs read triggers a new request, a new kthread is spawned
and immediately pops the abandoned request off the queue. When the response
arrives, the driver signals the new read's completion with the old read's
data, permanently shifting all subsequent reads.

> +	if (ret < 0) {
> +		dev_err(priv->client.dev, "error %d waiting for SMC call\n", ret);
> +		goto status_done;
> +	}
> +
> +	ret = 0;
> +
> +status_done:
> +	stratix10_svc_done(priv->chan);
> +	return ret;
> +}

[Severity: Medium]
Could this silently mask hardware failures?

If the transaction completes but the hardware rejects the command,
stratix10_hwmon_readtemp_cb() logs an error without modifying the cached
temperature or storing an error state.

This function then unconditionally returns 0 (success), causing the driver
to return the previously cached stale sensor value to user-space instead
of returning an error code.

[ ... ]

> +static void stratix10_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct stratix10_hwmon_priv *priv = platform_get_drvdata(pdev);
> +
> +	if (priv->async)
> +		stratix10_svc_remove_async_client(priv->chan);
> +	stratix10_svc_free_channel(priv->chan);
> +}

[Severity: High]
Can this result in a use-after-free crash during module unbind?

The sysfs attributes are registered using
devm_hwmon_device_register_with_info(), so they are removed by devres only
after this remove function returns.

Freeing the SVC channel manually here creates a race condition window where
the sysfs files are still active and readable, but the backend channel has
been destroyed. A concurrent sysfs read during driver unbinding will access
the freed channel.

Should the channel teardown be deferred using devm_add_action_or_reset()?

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