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 <[email protected]> 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 <[email protected]> Reviewed-by: Andy Shevchenko <[email protected]> Signed-off-by: Bartosz Golaszewski <[email protected]> > 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/[email protected]?part=13
