Re: [PATCH v12 12/14] accel/rocket: add RK3576 NPU (RKNN) support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver improperly tracks active RKNN cores using a sequential counter (`rdev->num_cores`) to index into the fixed-size `rdev->cores` array. Unbinding a core decrements the counter without shifting array elements, causing subsequent binds to overwrite active core structures and leading to skipped cleanup (use-after-free) and driver state corruption.
- [High] 64-bit IOVA task->regcmd is silently truncated to 32 bits, despite 40-bit DMA mask.
--

commit f9d20327e89caf3b9bf4ada2ba21cb2dc6201353
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: add RK3576 NPU (RKNN) support

This commit adds support for the RK3576 NPU by introducing new SoC-specific
definitions, clock/reset handling, PM domains, and adapting hardware task
control structures.

Link: https://lore.kernel.org/all/4f300b78-d96d-4d98-8819-dc292b0c9b97@rock-chips.com/

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -231,13 +231,23 @@ static void rocket_remove(struct platform_device *pdev)
>  static const struct rocket_soc_data rk3588_soc_data = {
>  	.num_clks = 4,
>  	.num_resets = 2,
> +	.multi_power_domain = false,
> +	.task_con_16bit = false,
>  };
>  
> -static const struct of_device_id dt_match[] = {
> +static const struct rocket_soc_data rk3576_soc_data = {
> +	.num_clks = 6,
> +	.num_resets = 1,
> +	.multi_power_domain = true,
> +	.task_con_16bit = true,
> +};
> +
> +const struct of_device_id rocket_dt_match[] = {
>  	{ .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data },
> +	{ .compatible = "rockchip,rk3576-rknn-core", .data = &rk3576_soc_data },
>  	{}
>  };
> -MODULE_DEVICE_TABLE(of, dt_match);
> +MODULE_DEVICE_TABLE(of, rocket_dt_match);

[Severity: Critical]
This isn't a bug introduced by this patch, but the way the driver tracks
active RKNN cores using rdev->num_cores appears to have a flaw that can
cause state corruption and a use-after-free.

If multiple cores are probed, and an intermediate core is unbound,
rocket_remove() decrements the counter without shifting the remaining
elements in the rdev->cores array to fill the gap:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
    rocket_core_fini(&rdev->cores[core]);
    rdev->num_cores--;
    ...
}

If another core is then probed, rocket_probe() allocates a slot based on the
decremented num_cores, causing the newly bound core to overwrite the
existing, active core structure:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
    unsigned int core = rdev->num_cores;
    dev_set_drvdata(&pdev->dev, rdev);
    rdev->cores[core].rdev = rdev;
    ...
}

When find_core_for_dev() is later called to unbind the overwritten core, it
fails to find it, which aborts rocket_remove() and leaks resources like the
running DRM scheduler thread. Eventually, when the driver is unloaded, the
memory containing the active thread is freed, leading to a use-after-free
crash.

Could we fix the core array management to handle unbinding properly?

[ ... ]
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -142,10 +166,17 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
>  	rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);
>  	rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1);
>  
> -	rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> -					 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> -					 PC_TASK_CON_TASK_NUMBER(1) |
> -					 PC_TASK_CON_TASK_PP_EN(1));
> +	if (core->soc->task_con_16bit)
> +		rocket_pc_writel(core, TASK_CON,
> +				 RK3576_PC_TASK_CON_LAST_LAYER_CLEAR |
> +				 RK3576_PC_TASK_CON_COUNT_CLEAR |
> +				 RK3576_PC_TASK_CON_PP_EN |
> +				 RK3576_PC_TASK_CON_TASK_NUMBER(1));
> +	else
> +		rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> +						 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> +						 PC_TASK_CON_TASK_NUMBER(1) |
> +						 PC_TASK_CON_TASK_PP_EN(1));

[Severity: High]
This is a pre-existing issue, but I noticed there is a potential 32-bit
truncation of a 64-bit IOVA in the hardware submission logic just above this
code in rocket_job_hw_submit().

The driver configures a 40-bit DMA mask in rocket_core_init():

drivers/accel/rocket/rocket_core.c:rocket_core_init() {
    ...
    err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
    ...
}

However, the UAPI struct restricts the IOVA to 32 bits:

include/uapi/drm/rocket_accel.h:drm_rocket_task {
    ...
    __u32 regcmd;
    ...
}

When userspace populates drm_rocket_task.regcmd, the __u32 type silently
truncates addresses greater than 4GB. The kernel then writes this truncated 
address to the 32-bit BASE_ADDRESS hardware register:

drivers/accel/rocket/rocket_job.c:rocket_job_hw_submit() {
    ...
    rocket_pc_writel(core, BASE_ADDRESS, task->regcmd);
    ...
}

Can this lead to IOMMU translation faults, device hangs, or memory corruption
if an unprivileged userspace process allocates enough memory to receive an
IOVA greater than 4GB?

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