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

[sashiko-bot@kernel.org]

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

--- 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709090153.21675-1-tze.yee.ng@altera.com?part=1