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

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 6/7/2026 10:58 pm, Guenter Roeck wrote:
> On 7/6/26 01:59, NG, TZE YEE wrote:
>> On 3/7/2026 4:18 pm, sashiko-bot@kernel.org wrote:
>>> 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;
>>>> +}
>>>
>>
>> Hi maintainers,
>>
>> 1. [High] Incorrect devm registration order causes a Use-After-Free
>> vulnerability during device unbind or probe failure.
>>
>> Agreed. v4 will register devm_add_action_or_reset() before
>> devm_hwmon_device_register_with_info(), matching other hwmon drivers
>> (e.g. pwm-fan, qnap-mcu-hwmon). On hwmon registration failure, probe
>> returns and devm runs the release action automatically, so manual
>> channel cleanup on that path is no longer needed.
>>
>> 2. [Medium] Unreferenced `completion` causes a guaranteed maximum
>> timeout delay on every asynchronous sensor read.
>>
>> Agreed that the completion is never signaled because the service layer
>> does not invoke the async callback. rsu_send_async_msg() in
>> stratix10-rsu.c uses the same send → wait → poll pattern, so we followed
>> RSU for consistency with the existing Stratix 10 async SVC client.
>>
> 
> Sorry, I am not willing to accept the "other code is buggy, therefore
> it is ok to introduce the same bug here" argument (Is that some new
> trend ? I hear it so often lately that I am getting tired of it).
> You'll have to provide a better argument explaining why it would make
> sense to call wait_for_completion_io_timeout() if it is known to always
> time out. You might as well just call usleep_range() or similar; while
> that would still be bad code, at least it would not be misleading.
> Just add a comment explaining the reason when you do that.
> 
> Guenter
> 

Hi Guenter,

You are right — arguing that RSU does the same thing is not a good
justification, and using wait_for_completion_io_timeout() on a 
completion that is never signaled is misleading.

After reviewing the stratix10-svc driver, we confirmed that the async
path does not invoke the callback registered in 
stratix10_svc_async_send(); results are obtained via
stratix10_svc_async_poll() instead. Completion-based waiting is only
appropriate for the synchronous path, where stratix10_svc_send()
delivers the response through receive_cb (which signals
priv->completion in socfpga_hwmon_sync_read()).

We will remove the local completion and pre-poll wait from
socfpga_hwmon_async_read() in v4. After stratix10_svc_async_send(), the
driver polls directly with the existing retry loop.

Thanks,
Tze Yee