Re: [PATCH 03/23] slimbus: qcom-ngd-ctrl: fix OF node refcount

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe error path bypasses cleanup, leaking PDR handle, SSR notifier, and leading to Use-After-Free.
- [High] The dynamically allocated child platform device is never unregistered upon module removal or driver unbind.
- [High] Hardware interrupt handler is registered before underlying structures are fully initialized.
- [High] Unsafe registration of the global platform driver within the controller's probe function.
- [High] Unchecked dereference of `of_match_node()` return value leads to a potential NULL pointer dereference.
- [High] SSR notifier can execute before work_struct initialization, causing a kernel crash.
- [High] Missing cancellation of `ngd_up_work` during device removal causes a Use-After-Free.
- [High] DMA coherent memory is leaked on every suspend/resume cycle.
--

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

slimbus: qcom-ngd-ctrl: fix OF node refcount

This commit fixes an OF node reference counting issue when using the split
approach for platform device creation. It adds a missing call to of_node_get()
before platform_device_add(), preventing an unbalanced of_node_put() when the
platform device is released.

> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index 1ed6be6e85d2c..428266949fdd0 100644
> --- a/drivers/slimbus/qcom-ngd-ctrl.c
> +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> @@ -1542,7 +1542,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,

[Severity: High]
This isn't a bug introduced by this patch, but in of_qcom_slim_ngd_register(),
the result of of_match_node() is immediately dereferenced.

If the parent device lacks a valid of_node (for example, if it is instantiated
manually or bound via sysfs), the match variable will be NULL. Could this
cause a kernel panic?

>  			kfree(ngd);
>  			return ret;
>  		}
> -		ngd->pdev->dev.of_node = node;
> +		ngd->pdev->dev.of_node = of_node_get(node);
>  		ctrl->ngd = ngd;
>  
>  		ret = platform_device_add(ngd->pdev);

[Severity: High]
This is a pre-existing issue, but of_qcom_slim_ngd_register() dynamically
allocates and adds a child platform device (ngd->pdev).

Do the controller removal function or the child device's unbind path ever
call platform_device_unregister() for this device?

If not, doesn't this cause memory and device leaks, permanently leak the
of_node reference, and prevent module reloading due to sysfs name collisions
in platform_device_add()?

[Severity: High]
This is a pre-existing issue, but if of_qcom_slim_ngd_register() fails, does
the probe function return the error directly and bypass the err_pdr_alloc and
err_pdr_lookup cleanup labels?

This would leave the SSR notifier registered. Because ctrl is managed by
devm_kzalloc(), it is freed upon probe failure. If the SSR notifier later
fires, won't it access the freed ctrl structure and lead to a Use-After-Free?
Additionally, are the cleanup labels incorrectly ordered?

[Severity: High]
This isn't a bug introduced by this patch, but in qcom_slim_ngd_ctrl_probe(),
devm_request_irq() registers qcom_slim_ngd_interrupt() before ctrl->ngd and
ctrl->ctrl.dev are assigned in the subsequent registration steps.

Could an early or spurious interrupt dereference these NULL pointers and lead
to a kernel panic?

[Severity: High]
This is a pre-existing issue, but platform_driver_register() is called for
qcom_slim_ngd_driver directly inside the controller's probe function.

Doesn't this break driver model semantics? If multiple controller instances
exist, subsequent registrations would fail. When any single controller
instance is removed, qcom_slim_ngd_ctrl_remove() unregisters this global
driver. Wouldn't that forcibly unbind all child devices across all active
controller instances?

[Severity: High]
This is a pre-existing issue, but the SSR notifier is registered in
qcom_slim_ngd_ctrl_probe() before the child device's probe function
initializes the ngd_up_work structure via INIT_WORK().

If the service is already UP, won't the notifier callback fire immediately
and call schedule_work(&ctrl->ngd_up_work) on a zero-initialized
work_struct, leading to a kernel crash?

[Severity: High]
This isn't a bug introduced by this patch, but the driver appears to fail to
call cancel_work_sync(&ctrl->ngd_up_work) in its remove path.

If an SSR event schedules this work on the system workqueue right before the
device is removed, wouldn't the work remain pending and execute after devm
has freed the ctrl structure, causing a Use-After-Free?

[Severity: High]
This is a pre-existing issue, but qcom_slim_ngd_init_dma() allocates DMA
memory for ctrl->rx_base and ctrl->tx_base using dma_alloc_coherent(), which
runs during setup and runtime resume.

However, the teardown function qcom_slim_ngd_exit_dma() only terminates the
DMA channels and never calls dma_free_coherent(). Doesn't this lead to a
repeated DMA memory leak on every suspend/resume cycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-pdev-fwnode-ref-v1-0-88c324a1b8d2@oss.qualcomm.com?part=3