Re: [PATCH v3 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device

[Dinh Nguyen <dinguyen@kernel.org>]

On 7/6/26 03:36, NG, TZE YEE wrote:
> On 3/7/2026 4:19 pm, sashiko-bot@kernel.org wrote:
>> 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++) {
>>
> 
> Hi,
> 
> Agreed with Sashiko's comment. In stratix10_svc_drv_remove(),
> stratix10_svc_async_exit() currently runs before the hwmon and RSU
> platform devices are unregistered. A concurrent sysfs read could still
> reach stratix10_svc_async_send() and use resources already torn down by
> stratix10_svc_async_exit().
> 
> This ordering predates the hwmon registration in this series; RSU
> follows the same pattern today. The fix would be to unregister child
> platform devices (and depopulate) before calling stratix10_svc_async_exit().
> 
> We can include that reorder in this series, or send it as a separate
> stratix10-svc fix.
> 

Please send that as a separate fix.

Thanks,
Dinh