Re: [RFC PATCH v4 4/6] accel/rocket: add RK3576 NPU (RKNN) support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Arbitrary physical memory corruption in experimental debug function rocket_sptr_patch_regcmd.
- [High] TOCTOU race in rocket_poll_work_fn leads to job corruption and premature completion.
- [High] Partial allocation failure causes NULL pointer dereference in rocket_snap_take.
- [High] Data race and memory leak on global variables in rocket_snap_take.
- [High] Hardware initialization is skipped after a job timeout reset, causing subsequent jobs to fail.
- [High] Teardown ordering bug: DRM scheduler is destroyed before canceling polling timer and work.
- [High] Resource leak on devm_pm_domain_attach_list error path.
- [Low] Logic error: rocket_core_reset hardcodes ARRAY_SIZE instead of utilizing the SoC-specific reset count.
--

--- Patch [4]: [RFC PATCH v4 4/6] accel/rocket: add RK3576 NPU (RKNN) support ---
commit 312454d18760cf447cabd81f08e885f92b5a4313
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: add RK3576 NPU (RKNN) support

This commit introduces support for the RK3576 NPU by abstracting platform
differences such as clock configurations, reset mechanisms, and power
domains. It also implements an hrtimer-based polling mechanism for job
completion since the RK3576 DPU completion interrupt does not reach the GIC.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index b3b2fa9ba645a..5738fccdbd133 100644
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
[ ... ]
> @@ -21,14 +49,22 @@ int rocket_core_init(struct rocket_core *core)
>  	/* RK3576 moves the BIU reset into its power domain and takes only srst_a. */
>  	core->resets[0].id = "srst_a";
>  	core->resets[1].id = "srst_h";
> -	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, ARRAY_SIZE(core->resets),
> +	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, core->soc->num_resets,
>  						    core->resets);

[Severity: Low]
Since we now fetch the resets using core->soc->num_resets, do we also need
to update rocket_core_reset() to use this count?

If rocket_core_reset() still uses ARRAY_SIZE(core->resets), it will try to
assert and deassert uninitialized elements on RK3576 where num_resets is 1:

rocket_core_reset() {
    reset_control_bulk_assert(ARRAY_SIZE(core->resets), core->resets);
    ...
}

>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
[ ... ]
> @@ -65,6 +101,19 @@ int rocket_core_init(struct rocket_core *core)
>  		return err;
>  	}
>  
> +	/*
> +	 * RK3576 spans two power domains, and a multi-domain device is skipped
> +	 * by the driver-core single-domain auto-attach, so attach the list here.
> +	 */
> +	if (core->soc->multi_power_domain) {
> +		struct dev_pm_domain_list *pd_list;
> +
> +		err = devm_pm_domain_attach_list(dev, NULL, &pd_list);
> +		if (err < 0)
> +			return dev_err_probe(dev, err,
> +					     "failed to attach NPU power domains\n");

[Severity: High]
Does this error path leak the resources allocated by rocket_job_init()?

Earlier in this function, rocket_job_init() successfully sets up non-devm
resources like the ordered workqueue (core->reset.wq), initializes the DRM
scheduler, and acquires a reference to core->iommu_group.

If devm_pm_domain_attach_list() fails, we return immediately. Do we need a
cleanup path here that calls rocket_job_fini() and releases the IOMMU
group?

> +	}
> +
>  	pm_runtime_use_autosuspend(dev);
[ ... ]
> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..95599e791e26f 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -240,12 +256,15 @@ static int rocket_device_runtime_resume(struct device *dev)
>  	if (core < 0)
>  		return -ENODEV;
>  
> -	err = clk_bulk_prepare_enable(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
> +	err = clk_bulk_prepare_enable(rdev->cores[core].soc->num_clks, rdev->cores[core].clks);
>  	if (err) {
>  		dev_err(dev, "failed to enable (%d) clocks for core %d\n", err, core);
>  		return err;
>  	}
>  
> +	/* Vendor runs its state_init once per power-up; we never did. */
> +	rocket_core_state_init(&rdev->cores[core]);

[Severity: High]
Does the timeout reset path also need to call this state initialization?

Looking at rocket_reset() which handles job timeouts, it calls
rocket_core_reset() to wipe the hardware state and then restarts the
scheduler.

Because the device does not go through a full suspend/resume cycle before the
next job executes, rocket_device_runtime_resume() is not triggered. Will
subsequent jobs fail because the hardware is left uninitialized?

> +
>  	return 0;
>  }
[ ... ]
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index bb77b6bf0f231..c21310ef78c7f 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> +static void rocket_snap_take(struct rocket_core *core)
> +{
> +	void __iomem *blk[ROCKET_SNAP_NBLK];
> +	unsigned int slot, b, i, w = 0, diffs = 0;
> +	u32 *cur;
> +
> +	if (!rocket_snap)
> +		return;
> +
> +	if (!rocket_snap_buf[0]) {

[Severity: High]
Are these global static variables safe from concurrent access?

RK3576 and RK3588 have multiple independent NPU cores. If two cores finish
jobs simultaneously and enter rocket_snap_take(), both might see
!rocket_snap_buf[0] as true. Could this result in double allocations and
leaked memory, as well as corrupted snapshot data from concurrent writes?

> +		rocket_snap_buf[0] = kmalloc_array(ROCKET_SNAP_WORDS, 4, GFP_KERNEL);
> +		rocket_snap_buf[1] = kmalloc_array(ROCKET_SNAP_WORDS, 4, GFP_KERNEL);
> +		if (!rocket_snap_buf[0] || !rocket_snap_buf[1])
> +			return;

[Severity: High]
Can this error path lead to a NULL pointer dereference on subsequent calls?

If rocket_snap_buf[0] allocates successfully but rocket_snap_buf[1] fails,
we return without freeing rocket_snap_buf[0].

The next time this function runs, !rocket_snap_buf[0] will be false, so it
skips the allocation block entirely. Later in the function, it will try to
use rocket_snap_buf[1], which is still NULL.

> +	}
> +
[ ... ]
> +static void rocket_sptr_patch_regcmd(struct rocket_core *core, struct rocket_job *job,
> +				     struct rocket_task *task, u32 bank)
> +{
> +	phys_addr_t phys = iommu_iova_to_phys(job->domain->domain, task->regcmd);
> +	unsigned int i, patched = 0;
> +	struct scatterlist sg;
> +	struct page *page;
> +	u64 *rv;
> +
> +	if (!phys || !pfn_valid(PFN_DOWN(phys)))
> +		return;
> +
> +	rv = phys_to_virt(phys);
> +	for (i = 0; i < task->regcmd_count; i++) {

[Severity: Critical]
Does this sequence allow userspace to overwrite arbitrary kernel physical
memory?

iommu_iova_to_phys() gives us a physical address, but phys_to_virt() is only
valid for direct-mapped memory, and the physical pages backing the IOVA are
not guaranteed to be contiguous.

Since task->regcmd_count is user-controlled, can an attacker provide a large
count that causes the loop to scan past the page boundary and overwrite memory
outside the intended buffer?

> +		u16 reg = rv[i] & 0xffff;
[ ... ]
> +static void rocket_poll_work_fn(struct work_struct *work)
> +{
> +	struct rocket_core *core = container_of(work, struct rocket_core, poll_work);
> +
> +	/*
> +	 * The interrupt can land while this work is already queued, finalise the
> +	 * job and let the next one start. Without this the stale work would then
> +	 * finalise that new job as well.
> +	 */
> +	if (core->poll_work_seq != core->poll_seq)
> +		return;
> +
> +	rocket_job_handle_irq(core);

[Severity: High]
Is this lockless sequence check vulnerable to a race condition?

If rocket_poll_work_fn() evaluates the condition to false but is preempted
immediately after, a physical IRQ could fire. That IRQ would complete the
current job and submit a new one (incrementing poll_seq).

When rocket_poll_work_fn() resumes, it will unconditionally execute
rocket_job_handle_irq(). Could this abruptly kill the newly submitted job
and prematurely signal its completion fence?

> +}
[ ... ]
> @@ -503,6 +803,12 @@ void rocket_job_fini(struct rocket_core *core)
>  {
>  	drm_sched_fini(&core->sched);
>  
> +	if (core->soc->poll_completion) {
> +		atomic_set(&core->poll_active, 0);
> +		hrtimer_cancel(&core->poll_timer);
> +		cancel_work_sync(&core->poll_work);
> +	}

[Severity: High]
Should the polling timer and work be stopped before destroying the scheduler?

drm_sched_fini() halts the scheduler, but any already-queued poll_work can
still execute immediately after. If it does, rocket_job_handle_irq() might
submit the next task to the hardware.

As teardown continues, clocks are disabled and memory is unmapped. If the NPU
is actively executing a newly started job when this happens, could it cause
bus hangs or IOMMU faults?

> +
>  	cancel_work_sync(&core->reset.work);
>  	destroy_workqueue(core->reset.wq);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803094125.3285895-1-gahing@gahingwoo.com?part=4