[PATCH] accel/rocket: clear rdev on device init failure

[PATCH] accel/rocket: clear rdev on device init failure

[Guangshuo Li]

rocket_probe() stores the return value from rocket_device_init() in the
file-scoped rdev pointer before checking whether it is an error pointer.
If rocket_device_init() fails, rdev is left as an ERR_PTR.

The rdev pointer is also used as the marker for whether the shared rocket
device has already been initialized. A later core probe can therefore see
a non-NULL rdev, skip rocket_device_init(), and dereference the error
pointer when reading rdev->num_cores.

Save the error code and clear rdev before returning from the
rocket_device_init() failure path.

Fixes: ed98261b4168 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/accel/rocket/rocket_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
index 8bbbce594883..67e7f543fe4c 100644
--- a/drivers/accel/rocket/rocket_drv.c
+++ b/drivers/accel/rocket/rocket_drv.c
@@ -165,8 +165,11 @@ static int rocket_probe(struct platform_device *pdev)
 		/* First core probing, initialize DRM device. */
 		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
 		if (IS_ERR(rdev)) {
+			int err = PTR_ERR(rdev);
+
 			dev_err(&pdev->dev, "failed to initialize rocket device\n");
-			return PTR_ERR(rdev);
+			rdev = NULL;
+			return err;
 		}
 	}
 
-- 
2.43.0

Re: [PATCH] accel/rocket: clear rdev on device init failure

[sashiko-bot]

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

Pre-existing issues:
- [Critical] NULL pointer dereference when opening the DRM device before core initialization completes.
- [Critical] Use-After-Free due to deferred DRM device unregistration during individual core teardown.
- [High] Massive memory and resource leak due to devres allocations on a permanent platform device.
- [High] Out-of-bounds access and data corruption due to missing array compaction on core removal.
- [High] Race condition and data races on global state during concurrent core probes.
- [High] Memory leak of IOMMU domain reference in `rocket_ioctl_create_bo` error path.
- [High] Use-After-Free and NULL pointer dereference in concurrent timeout handler during teardown.
- [High] Missing clock IDs in `clk_bulk_data` causes failure to properly request and enable hardware clocks.
--

--- Patch [1]: [PATCH] accel/rocket: clear rdev on device init failure ---
commit 17e3337636f75d45b7220a5f2dd42ebed6a265c7
Author: Guangshuo Li <lgs201920130244@gmail.com>

accel/rocket: clear rdev on device init failure

This patch updates rocket_probe() to clear the file-scoped rdev pointer and
return the saved error code if rocket_device_init() fails. This prevents a
later probe from seeing a non-NULL rdev and dereferencing an error pointer.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..67e7f543fe4c7 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -165,8 +165,11 @@ static int rocket_probe(struct platform_device *pdev)
>  		/* First core probing, initialize DRM device. */
>  		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
>  		if (IS_ERR(rdev)) {
> +			int err = PTR_ERR(rdev);
> +
>  			dev_err(&pdev->dev, "failed to initialize rocket device\n");
> -			return PTR_ERR(rdev);
> +			rdev = NULL;

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak
memory?

In drivers/accel/rocket/rocket_device.c:rocket_device_init(), allocations
are made using devres bound to the persistent drm_dev:

    rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device,
                              ddev);

Since drm_dev is a static platform device that persists until module unload,
these devres allocations are never freed. If rocket_probe() fails here,
abandoning the devm-managed allocations without freeing them, will successive
probe failures or unbind/bind cycles continuously stack unbounded memory leaks?

> +			return err;
>  		}
>  	}
>  

[Severity: Critical]
This isn't a bug introduced by this patch, but could this cause a NULL
pointer dereference when opening the DRM device?

In drivers/accel/rocket/rocket_device.c:rocket_device_init(), the device is
registered and exposed to userspace prematurely:

    err = drm_dev_register(ddev, 0);

If an unprivileged user opens the DRM device right after registration, before
rocket_probe() finishes initializing the hardware cores, rocket_open() is
called:

    rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev);

Since cores[0].dev is still NULL, this passes a NULL device pointer to
iommu_paging_domain_alloc(), which would cause a kernel panic when it
dereferences dev->iommu.

[Severity: Critical]
This is a pre-existing issue, but does deferred DRM device unregistration
lead to a use-after-free during individual core teardown?

When an individual core is unbound,
drivers/accel/rocket/rocket_drv.c:rocket_remove() destroys that core's
drm_gpu_scheduler:

    rocket_core_fini(&rdev->cores[core]);

However, drm_dev_unregister() is deferred until the very last core is
removed. Open file descriptors can still access the device, and
drivers/accel/rocket/rocket_job.c:rocket_job_open() will initialize a
drm_sched_entity pointing to the destroyed scheduler:

    scheds[core] = &rdev->cores[core].sched;

Could subsequent user ioctls to submit jobs push them to a destroyed
scheduler, causing a use-after-free crash?

[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds
access due to missing array compaction on core removal?

Removing a core decrements the count without compacting the array:

    rdev->num_cores--;

As a result, find_core_for_dev() iterations will truncate and fail to locate
active cores at higher indices. Furthermore, if a new core is probed,
drivers/accel/rocket/rocket_drv.c:rocket_probe() uses the artificially
lowered num_cores as the index:

    rdev->cores[core].rdev = rdev;

Would this overwrite an existing active core slot, destroying its state and
corrupting the array mapping?

[Severity: High]
This is a pre-existing issue, but is there a data race on global state
during concurrent core probes?

The global rdev pointer and rdev->num_cores counter are accessed and
modified without synchronization. Concurrent probes for different device
tree nodes could race here in
drivers/accel/rocket/rocket_drv.c:rocket_probe():

    if (rdev == NULL) {

and later increment without synchronization:

    rdev->num_cores++;

Could multiple threads simultaneously pass the NULL check, leading to
duplicate memory allocations, corrupting the rdev->cores array, and losing
core registrations?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
rocket_ioctl_create_bo leak the IOMMU domain reference?

If BO creation fails, the error path directly calls:

    drm_gem_shmem_object_free(gem_obj);

This frees the GEM object but completely bypasses the driver's custom
rocket_gem_bo_free() callback. As a result, the IOMMU domain reference
acquired earlier is never released:

    rkt_obj->domain = rocket_iommu_domain_get(rocket_priv);

Could a malicious userspace program exploit this to cause an out of memory
condition by repeatedly triggering mapping failures?

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free and
NULL pointer dereference in a concurrent timeout handler?

During unbind, drivers/accel/rocket/rocket_core.c:rocket_core_fini() drops
the IOMMU group reference and clears the pointer before tearing down the
scheduler:

    core->iommu_group = NULL;

If a job timeout triggers concurrently,
drivers/accel/rocket/rocket_job.c:rocket_reset() is scheduled and calls:

    iommu_detach_group(NULL, core->iommu_group);

Does this dereference the now-NULL core->iommu_group pointer inside
iommu_detach_group?

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of clock IDs
cause a failure to properly request and enable hardware clocks?

In drivers/accel/rocket/rocket_core.c:rocket_core_init(), clocks are fetched
using an uninitialized array where the .id fields are NULL:

    err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);

When .id is NULL, the clock subsystem falls back to fetching the very first
clock listed in the device tree for the device. Does this result in the
driver obtaining four references to the exact same clock instead of the four
distinct required clocks, causing hardware functional failure during power
management?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708062845.716487-1-lgs201920130244@gmail.com?part=1

Re: [PATCH] accel/rocket: clear rdev on device init failure

[Igor Paunovic]

Hi Guangshuo,

I have been working on this driver on RK3588 (Orange Pi 5 Plus) and
went through your fix carefully:

- The analysis matches the code: rocket_probe() publishes the ERR_PTR
  into the file-scoped rdev before the IS_ERR() check, and since
  __driver_attach() keeps probing the remaining devices after a failed
  probe, the next rknn-core dereferences the poisoned pointer at
  rdev->num_cores. Every RK3588 DT carries three rknn-core nodes, so
  the follow-up probe is guaranteed.

- rocket_device_init() cannot return -EPROBE_DEFER (it does no clock,
  regulator or supplier lookups), so clearing rdev here cannot
  interfere with the deferral path in rocket_core_init(), which
  already resets rdev correctly further down in rocket_probe().

- The fix mirrors the shape the driver already uses on the
  rocket_core_init() failure path (fini + rdev = NULL) and changes
  nothing on the success path.

Reviewed-by: Igor Paunovic <royalnet026@gmail.com>

I also ran the patched driver on RK3588: normal three-core probe,
runtime PM suspend/resume and a Teflon/MobileNetV1 inference run are
all unaffected.

Thanks,
Igor