Re: [PATCH 8/8] slimbus: qcom-ngd-ctrl: use platform_device_set_fwnode()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing NULL check on `of_match_node()` result leads to NULL pointer dereference.
- [High] Premature freeing of parent controller's data in child driver's remove callback causes NULL pointer dereference.
- [High] Use-After-Free due to uncancelled work on the system workqueue.
--

--- Patch [8]: [PATCH 8/8] slimbus: qcom-ngd-ctrl: use platform_device_set_fwnode() ---
commit 1bfb7c41dded376cdac1bca23cdc026328648166
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

slimbus: qcom-ngd-ctrl: use platform_device_set_fwnode()
    
Prefer the higher-level platform_device_set_fwnode() over the
OF-specific platform_device_set_of_node() for dynamically allocated
platform devices.

> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index 934c44c5bc1a9..bf077f47c9ddd 100644
> --- a/drivers/slimbus/qcom-ngd-ctrl.c
> +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> @@ -1538,7 +1538,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,
>  			kfree(ngd);
>  			return ret;
>  		}
> -		platform_device_set_of_node(ngd->pdev, node);
> +		platform_device_set_fwnode(ngd->pdev, of_fwnode_handle(node));
>  		ctrl->ngd = ngd;
>  
>  		ret = platform_device_add(ngd->pdev);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at
of_qcom_slim_ngd_register(), I noticed that of_match_node() doesn't
have a NULL check:

drivers/slimbus/qcom-ngd-ctrl.c:of_qcom_slim_ngd_register() {
	...
	match = of_match_node(qcom_slim_ngd_dt_match, parent->of_node);
	data = match->data;
	...
}

Can this lead to a NULL pointer dereference if a user with root privileges
binds the driver via sysfs to a platform device that lacks an appropriate
of_node?

[Severity: High]
This is a pre-existing issue, but I noticed the parent controller's data
could be prematurely freed in the child driver's remove callback:

drivers/slimbus/qcom-ngd-ctrl.c:qcom_slim_ngd_remove() {
	...
	kfree(ctrl->ngd);
	ctrl->ngd = NULL;
	...
}

If the child driver is unbound via sysfs, this frees ctrl->ngd. If the
parent driver is subsequently unbound, won't qcom_slim_ngd_unregister()
dereference the NULL ctrl->ngd pointer to delete the platform device?

drivers/slimbus/qcom-ngd-ctrl.c:qcom_slim_ngd_unregister() {
	struct qcom_slim_ngd *ngd = ctrl->ngd;

	platform_device_del(ngd->pdev);
}

[Severity: High]
This is also a pre-existing issue, but could there be a use-after-free
if work is scheduled on the system workqueue during driver removal?

In qcom_slim_ngd_ctrl_remove(), the private workqueue is destroyed, but
cancel_work_sync(&ctrl->ngd_up_work) is omitted for the system workqueue:

drivers/slimbus/qcom-ngd-ctrl.c:qcom_slim_ngd_ctrl_remove() {
	...
	destroy_workqueue(ctrl->mwq);
	...
}

If the scheduled work executes after devres frees the ctrl structure, won't
the worker dereference the freed memory?

drivers/slimbus/qcom-ngd-ctrl.c:qcom_slim_ngd_up_worker() {
	...
	ctrl = container_of(work, struct qcom_slim_ngd_ctrl, ngd_up_work);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-pdev-set-fwnode-instead-of-of-node-v1-0-2dee93f42c54@oss.qualcomm.com?part=8