Re: [PATCH v3 13/20] slimbus: qcom-ngd-ctrl: use platform_device_set_of_node()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The dynamically allocated platform device and its associated OF node reference are leaked during device unregistration.
- [High] Unconditional dereference of `of_match_node()` return value leads to a NULL pointer dereference.
- [High] Missing cancellation of asynchronous work `ngd_up_work` causes a Use-After-Free of the `ctrl` structure on module removal.
--

commit 4a2753767e946e9e44740066e0bb9147c741d781
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

slimbus: qcom-ngd-ctrl: use platform_device_set_of_node()

Ahead of reworking the reference counting logic for platform devices,
encapsulate the assignment of the OF node for dynamically allocated
platform devices with the provided helper.

Acked-by: Konrad Dybcio <konradybcio@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index 3071e46d03bea..6e89415712ce1 100644
> --- a/drivers/slimbus/qcom-ngd-ctrl.c
> +++ b/drivers/slimbus/qcom-ngd-ctrl.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can this code cause a NULL
pointer dereference?

Looking at the beginning of of_qcom_slim_ngd_register():

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;

If the driver is bound via sysfs to a platform device that lacks a device
tree node, of_match_node() will return NULL. Should this use
of_device_get_match_data() or validate the match pointer before accessing
data?

> @@ -1539,7 +1539,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,
>  			kfree(ngd);
>  			return ret;
>  		}
> -		ngd->pdev->dev.of_node = of_node_get(node);
> +		platform_device_set_of_node(ngd->pdev, node);
>  		ctrl->ngd = ngd;
>  
>  		ret = platform_device_add(ngd->pdev);

[Severity: High]
This is a pre-existing issue, but does this dynamically allocated device
get leaked when unregistered?

Looking at qcom_slim_ngd_unregister() in drivers/slimbus/qcom-ngd-ctrl.c:

qcom_slim_ngd_unregister() {
    ...
    platform_device_del(ngd->pdev);
    ...
}

Since of_qcom_slim_ngd_register() allocates the device with
platform_device_alloc(), shouldn't platform_device_put() be called to drop
the initial reference count? Without it, both the device structure and the
device tree node reference acquired above are never freed during unbind.

[Severity: High]
This is a pre-existing issue, but is there a risk of a use-after-free
during module removal?

Looking at the driver teardown in qcom_slim_ngd_ctrl_remove():

drivers/slimbus/qcom-ngd-ctrl.c:qcom_slim_ngd_ctrl_remove() {
    ...
    qcom_slim_ngd_unregister(ctrl);

    destroy_workqueue(ctrl->mwq);
}

If qcom_slim_ngd_ssr_pdr_notify() has scheduled ctrl->ngd_up_work on the
system workqueue, can it execute after the devres-managed ctrl structure
is freed? Would adding cancel_work_sync(&ctrl->ngd_up_work) here ensure
the background task completes before the memory is released?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-pdev-fwnode-ref-v3-0-1ff028e33779@oss.qualcomm.com?part=13