[PATCH RESEND] accel/rocket: Validate task regcmd address and count on submission

[PATCH RESEND] accel/rocket: Validate task regcmd address and count on submission

[Sidong Yang]

The regcmd fields in drm_rocket_task come from userspace and are
programmed into the PC unit without any validation.

Bits 31:4 of PC_BASE_ADDRESS hold the register command DMA address
and bit 0 selects slave mode, so a misaligned regcmd silently drops
its low bits, and an odd address flips the PC unit into slave mode.
Similarly, pc_data_amount is a 16-bit field holding
(regcmd_count + 1) / 2 - 1, so a larger regcmd_count is silently
truncated by the register encoding.

Reject unaligned regcmd addresses and out-of-range regcmd_count with
-EINVAL at submission time. Existing userspace is not affected: Mesa
places regcmd buffers at 64-byte aligned offsets, and its regcmd
counts stay far below the limit.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
Resending unchanged since the July 11 submission.
Applies cleanly to drm-misc-next at
9d4c98219598e77ca7044c453ae6994519ac0a96.

Previous submission:
https://lore.kernel.org/dri-devel/20260711062137.36044-1-sidong.yang@furiosa.ai/

A review would be appreciated.

 drivers/accel/rocket/rocket_job.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index bb77b6bf0f23..9ebabf86fe97 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -7,6 +7,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
 #include <drm/rocket_accel.h>
+#include <linux/align.h>
 #include <linux/interrupt.h>
 #include <linux/overflow.h>
 #include <linux/iommu.h>
@@ -21,6 +22,15 @@
 
 #define JOB_TIMEOUT_MS 500
 
+/*
+ * The PC unit fetches two 64-bit register commands per pc_data_amount unit,
+ * and the field holds (regcmd_count + 1) / 2 - 1.
+ */
+#define ROCKET_MAX_REGCMDS	((PC_REGISTER_AMOUNTS_PC_DATA_AMOUNT__MASK + 1) * 2U)
+
+/* Bits 3:0 of PC_BASE_ADDRESS hold the mode selection bit and reserved bits */
+#define ROCKET_REGCMD_ALIGN	16
+
 static struct rocket_job *
 to_rocket_job(struct drm_sched_job *sched_job)
 {
@@ -95,6 +105,20 @@ rocket_copy_tasks(struct drm_device *dev,
 			goto fail;
 		}
 
+		if (task.regcmd_count > ROCKET_MAX_REGCMDS) {
+			drm_dbg(dev, "regcmd_count field in drm_rocket_task should be <= %u.\n",
+				ROCKET_MAX_REGCMDS);
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		if (!IS_ALIGNED(task.regcmd, ROCKET_REGCMD_ALIGN)) {
+			drm_dbg(dev, "regcmd field in drm_rocket_task should be aligned to %u bytes.\n",
+				ROCKET_REGCMD_ALIGN);
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		rjob->tasks[i].regcmd = task.regcmd;
 		rjob->tasks[i].regcmd_count = task.regcmd_count;
 	}

base-commit: a284476db2653ae893f46cbea408eb412db54eb0
-- 
2.42.0

Re: [PATCH RESEND] accel/rocket: Validate task regcmd address and count on submission

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Integer overflow in the loop index of `rocket_copy_tasks` causes a massive out-of-bounds kernel memory write.
- [High] `rocket_ioctl_submit` completely ignores the return value of `rocket_ioctl_submit_job`, causing silent failures.
- [High] `rocket_job_init` calls `drm_sched_fini` on an uninitialized scheduler if `drm_sched_init` fails.
- [High] `rocket_job_open` causes a NULL pointer dereference if `kmalloc_objs` fails, and leaks memory on entity init failure.
- [High] The IRQ handler leaks an IOMMU group reference by calling `iommu_group_get()` without a matching `iommu_group_put()`.
--

commit 6a2dfc1f11654f17ccebcb06bd5abfedbf9516c0
Author: Sidong Yang <sidong.yang@furiosa.ai>

accel/rocket: Validate task regcmd address and count on submission

This commit adds validation for the regcmd fields in drm_rocket_task
that come from userspace before they are programmed into the PC unit.
It rejects unaligned regcmd addresses and out-of-range regcmd_count
with -EINVAL at submission time to prevent hardware malfunctions.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index bb77b6bf0f231..9ebabf86fe97f 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -95,6 +105,20 @@ rocket_copy_tasks(struct drm_device *dev,
>  			goto fail;
>  		}
>  

[Severity: Critical]
This is a pre-existing issue, but does this code overflow the rjob->tasks
array?

Looking at rocket_copy_tasks():

    rjob->tasks = kvmalloc_objs(*rjob->tasks, job->task_count);
    ...
    for (int i = 0; i < rjob->task_count; i++) {

If a user calls the submission ioctl with job->task_count greater than
0x80000000, and there is enough memory to allocate the array, the signed 32-bit
int i loop index reaches -2147483648. The loop condition implicitly casts
this to unsigned, allowing it to bypass the check.

The array index rjob->tasks[i] then sign-extends the negative index to a 64-bit
pointer offset:

    rjob->tasks[i].regcmd = task.regcmd;
    rjob->tasks[i].regcmd_count = task.regcmd_count;

Could this lead to an out-of-bounds write of user-controlled data to
kernel memory?

> +		if (task.regcmd_count > ROCKET_MAX_REGCMDS) {
> +			drm_dbg(dev, "regcmd_count field in drm_rocket_task should be <= %u.\n",
> +				ROCKET_MAX_REGCMDS);
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
> +		if (!IS_ALIGNED(task.regcmd, ROCKET_REGCMD_ALIGN)) {
> +			drm_dbg(dev, "regcmd field in drm_rocket_task should be aligned to %u bytes.\n",
> +				ROCKET_REGCMD_ALIGN);
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
>  		rjob->tasks[i].regcmd = task.regcmd;
>  		rjob->tasks[i].regcmd_count = task.regcmd_count;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but are we dropping the return
value of rocket_ioctl_submit_job() here?

In rocket_ioctl_submit():

    for (i = 0; i < args->job_count; i++)
        rocket_ioctl_submit_job(dev, file, &jobs[i]);

If userspace submits a job that violates the validation checks added in this
patch (returning -EINVAL), or fails memory allocation (-ENOMEM), the error
appears to be discarded. The ioctl returns 0 to userspace, but the job is not
queued and no completion fences are signaled. Should this return the error
code instead?

[Severity: High]
This is a pre-existing issue, but does rocket_job_init() tear down an
uninitialized scheduler on error?

    ret = drm_sched_init(&core->sched, &args);
    if (ret) {
        dev_err(core->dev, "Failed to create scheduler: %d.", ret);
        goto err_sched;
    }
    return 0;

err_sched:
    drm_sched_fini(&core->sched);

If drm_sched_init() fails, it cleans up after itself and returns an error.
Calling drm_sched_fini() on the same scheduler could attempt to tear down
structures that were not successfully initialized, potentially causing a crash
during device initialization. Can this error path be fixed?

[Severity: High]
This is a pre-existing issue, but does rocket_job_open() leak memory or
dereference a NULL pointer?

    struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
                                                     rdev->num_cores);
    unsigned int core;
    int ret;

    for (core = 0; core < rdev->num_cores; core++)
        scheds[core] = &rdev->cores[core].sched;

If kmalloc_objs() fails and returns NULL, the loop immediately dereferences the
pointer.

Also, if drm_sched_entity_init() fails later:

    ret = drm_sched_entity_init(&rocket_priv->sched_entity, ...);
    if (WARN_ON(ret))
        return ret;

It seems the function returns early without freeing the allocated scheds array.
Should the return value of kmalloc_objs be checked and the array freed on
error?

[Severity: High]
This is a pre-existing issue, but does rocket_job_handle_irq() leak an IOMMU
group reference?

    iommu_detach_group(NULL, iommu_group_get(core->dev));

When the hardware signals job completion, iommu_group_get() is called to pass
to iommu_detach_group(). Since iommu_detach_group() does not consume the
reference and no iommu_group_put() is called, the reference appears to be
leaked on every single job completion.

Could this quickly cause the refcount to saturate, permanently pinning the
memory and device structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178921240194.10.15893724064993644797.rocket-regcmd-resend@furiosa.ai?part=1