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

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

On 9/7/2026 5:11 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The driver removal sequence incorrectly destroys the async transaction backend before unregistering the child client devices, leading to potential Use-After-Free and NULL pointer dereferences.

The fix is implemented in separate patch: 
https://git.kernel.org/pub/scm/linux/kernel/git/dinguyen/linux.git/commit/?h=socfpga_svc_fixes_for_v7.2&id=bba85375f8751d6c949036269b529cdcb2a2ed8f

In v5, we will rebase on dinguyen/socfpga_svc_fixes_for_v7.2 to pick up 
this fix.

> - [High] ida_alloc_max() is called with an inclusive maximum of 16, which overflows the 4-bit bitfields for client and job IDs.

Agreed. ida_alloc_max() with max 16 is inclusive, but the job and client
IDs are packed into 4-bit fields (STRATIX10_JOB_FIELD /
STRATIX10_CLIENT_FIELD), so allocating ID 16 truncates to 0 and can
corrupt ida_free().

We plan to fix this in the same separate stratix10-svc series by 
limiting ida_alloc_max() to 15 for both job and client ID pools.

> --
> 
> --- Patch [1]: [PATCH v4 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device ---
> commit 525d10487a60f1d84c17127378990a4a611c8881
> Author: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device
>      
> This commit adds asynchronous Stratix 10 service layer support for hardware
> monitor temperature and voltage read commands. It also registers 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..716b58f5a6170 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
> [ ... ]
>> @@ -1329,6 +1332,14 @@ int stratix10_svc_async_send(struct stratix10_svc_chan *chan, void *msg,
>>   		args.a0 = INTEL_SIP_SMC_ASYNC_RSU_NOTIFY;
>>   		args.a2 = p_msg->arg[0];
>>   		break;
>> +	case COMMAND_HWMON_READTEMP:
>> +		args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READTEMP;
>> +		args.a2 = p_msg->arg[0];
>> +		break;
>> +	case COMMAND_HWMON_READVOLT:
>> +		args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READVOLT;
>> +		args.a2 = p_msg->arg[0];
>> +		break;
>>   	default:
>>   		dev_err(ctrl->dev, "Invalid command ,%d\n", p_msg->command);
>>   		ret = -EINVAL;
> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch, but does
> the job ID allocation earlier in stratix10_svc_async_send() wrap around when
> assigning the maximum ID?
> 
> In stratix10_svc_async_send():
> 	ret = ida_alloc_max(&achan->job_id_pool, MAX_SDM_JOB_IDS,
> 			    GFP_KERNEL);
> 
> ida_alloc_max() can allocate an ID up to MAX_SDM_JOB_IDS (16) inclusive.
> Because both the job ID and client ID are stored in 4-bit bitfields,
> allocating ID 16 (0x10) will be truncated to 0 when packed.
> 
> When the transaction completes, ida_free() would receive ID 0 instead of 16,
> which could unexpectedly free an active job's ID while ID 16 is leaked.
> A similar issue appears in stratix10_svc_add_async_client() with
> MAX_SDM_CLIENT_IDS.
> 
> [ ... ]
>> @@ -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);
>> +
>>   	platform_device_unregister(svc->stratix10_svc_rsu);
>>   
>>   	for (i = 0; i < SVC_NUM_CHANNEL; i++) {
> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch, but does
> this driver removal sequence destroy the async transaction backend before
> unregistering the child devices?
> 
> Looking at stratix10_svc_drv_remove():
> 
> 	stratix10_svc_async_exit(ctrl);
> 	of_platform_depopulate(ctrl->dev);
> 	if (svc->stratix10_svc_hwmon)
> 		platform_device_unregister(svc->stratix10_svc_hwmon);
> 	platform_device_unregister(svc->stratix10_svc_rsu);
> 
> Because stratix10_svc_async_exit() is called first, it frees all active
> transaction handlers and sets actrl->invoke_fn to NULL. However, the child
> devices (socfpga-hwmon, stratix10-rsu) are still registered and could initiate
> or poll requests during this window.
> 
> Could concurrent accesses to the child devices trigger a use-after-free on
> handler->next or a NULL pointer dereference on actrl->invoke_fn?
>