Re: [PATCH 16/23] slimbus: qcom-ngd-ctrl: use platform_device_set_of_node()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Probe error path bypasses cleanup labels, leaking resources and leaving dangling notifiers.
- [Critical] Early IRQ request allows a race condition leading to a NULL pointer dereference or access to uninitialized locks in the interrupt handler.
- [Critical] PDR/SSR notifiers are registered before their corresponding work structs are initialized, leading to potential panics on early events.
- [High] Unchecked return value from of_match_node() causes NULL pointer dereference.
- [High] Child device remove callback frees parent device resources, leading to Use-After-Free and resource leaks.
- [High] Registering a platform driver from within a platform device probe callback.
- [High] Error handling labels are in reversed order, causing invalid pointer accesses and incomplete cleanup.
- [High] Dangling `ctrl->ngd` pointer is left behind on `platform_device_add()` failure.
- [High] The driver fails to cancel `ctrl->ngd_up_work` during removal, leading to a Use-After-Free if the work is pending.
- [Medium] Invalid pointer arithmetic causing out-of-bounds MMIO mapping if child reg property is 0.
--

commit 764709448d03c52a2585b5617fd60c1746e5cc5a
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.

> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index 428266949fdd0..70f0698153323 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,
>  			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 is a dangling ctrl->ngd pointer left behind
on platform_device_add() failure?

During of_qcom_slim_ngd_register(), ctrl->ngd is assigned before
platform_device_add() is called. If adding the device fails, kfree(ngd) is
called, but ctrl->ngd is never reset to null.

Since the irq and ssr notifier are already active, if either fires before the
ctrl structure is eventually destroyed, they will dereference the freed ngd
pointer, resulting in a use-after-free.

[Severity: High]
This is a pre-existing issue, but does this lack a null check?

In of_qcom_slim_ngd_register(), of_match_node() is called with
parent->of_node, and its return value is immediately dereferenced via
match->data. If the device is instantiated without a device tree (e.g., via
sysfs manual bind) or if the match fails, of_match_node() returns null.
Dereferencing it without a null check causes a kernel panic.

[Severity: Critical]
This is a pre-existing issue, but does the probe error path bypass cleanup
labels?

In qcom_slim_ngd_ctrl_probe(), if of_qcom_slim_ngd_register() fails (e.g.
returning -ENODEV when no child nodes exist), the function directly executes
return of_qcom_slim_ngd_register(...).

This bypasses the error cleanup labels err_pdr_alloc and err_pdr_lookup,
failing to unregister the ssr notifier and free the pdr handle. Since the ctrl
structure is devres-managed, it is freed upon probe failure. A subsequent ssr
notification will then dereference the freed ctrl object, leading to a
use-after-free.

[Severity: High]
This is a pre-existing issue, but does the child device remove callback free
parent device resources?

The parent device's probe (qcom_slim_ngd_ctrl_probe) allocates ctrl->pdr and
registers ctrl->notifier. However, the child device's remove callback
(qcom_slim_ngd_remove) frees them.

If a user unbinds the child device via sysfs, these resources are freed while
the parent is still active. If the child is bound again, or if an ssr event
triggers, it will access the freed resources. Furthermore, if no child is ever
added or probed, these resources are never freed when the parent is removed.

[Severity: High]
This is a pre-existing issue, but is it correct to register a platform driver
from within a platform device probe callback?

qcom_slim_ngd_ctrl_probe() calls platform_driver_register() during its own
probe. This violates the Linux device driver model. If there are multiple
instances of the qcom,slim-ngd-ctrl controller, the second probe will fail
to register the driver but ignore the error.

Additionally, the parent's remove callback unregisters the driver globally,
forcefully unbinding all ngd child devices across all controller instances.

[Severity: Medium]
This is a pre-existing issue, but can invalid pointer arithmetic cause an
out-of-bounds mmio mapping if the child reg property is 0?

If a child node specifies reg = <0>, ngd->id becomes 0. The expression
(ngd->id - 1) * data->size multiplies -1 by a u32. Due to type promotion,
this wraps to a huge unsigned value (0xffffffff * data->size), resulting in
an out-of-bounds mmio address calculation when added to the ctrl->base
pointer.

[Severity: High]
This is a pre-existing issue, but are the error handling labels in reversed
order?

In qcom_slim_ngd_ctrl_probe(), the cleanup labels are in the wrong order. If
pdr_add_lookup() fails, the code jumps to err_pdr_lookup, completely skipping
qcom_unregister_ssr_notifier(), leaking the notifier and opening a
use-after-free window.

If pdr_handle_alloc() fails, it jumps to err_pdr_alloc, unregisters the
notifier, and falls through to err_pdr_lookup, calling pdr_handle_release()
on an IS_ERR pointer.

[Severity: Critical]
This is a pre-existing issue, but does the early irq request allow a race
condition?

In qcom_slim_ngd_ctrl_probe(), devm_request_irq() is called before
of_qcom_slim_ngd_register() initializes the ctrl->ngd pointer, and before
ctrl->ctrl.dev and locks are initialized.

If an early or pending hardware interrupt fires during this window,
qcom_slim_ngd_interrupt() will execute and immediately dereference
ctrl->ngd->base (which is null) or call pm_runtime_suspended(ctrl->ctrl.dev)
(also null), leading to a null pointer dereference.

[Severity: Critical]
This is a pre-existing issue, but are pdr and ssr notifiers registered before
their corresponding work structs are initialized?

The ssr notifier and pdr handle are registered in the parent controller probe,
immediately exposing the driver to notifications. The handler
qcom_slim_ngd_ssr_pdr_notify() calls schedule_work(&ctrl->ngd_up_work).

However, ctrl->ngd_up_work is initialized much later in qcom_slim_ngd_probe()
(the child driver's probe). If an event arrives before the child driver
probes, the kernel will enqueue an uninitialized work_struct, causing
workqueue list corruption and a kernel panic.

[Severity: High]
This is a pre-existing issue, but does the driver fail to cancel
ctrl->ngd_up_work during removal?

The driver schedules ctrl->ngd_up_work on the global system workqueue via
schedule_work(). However, neither qcom_slim_ngd_remove() nor
qcom_slim_ngd_ctrl_remove() calls cancel_work_sync(&ctrl->ngd_up_work) before
returning.

The memory for ctrl will then be freed by devres. If the scheduled work
executes after the memory is freed, it results in a use-after-free.

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