Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [Critical] Signed integer overflow in loop counter allows massive out-of-bounds kernel memory write. - [High] Silent failure of job submission due to ignored return value. - [High] IOMMU group reference leak on every job completion. - [High] Hardware incompatibility due to 32-bit DMA address truncation for IOMMU allocations. - [High] Unprivileged userspace can trigger a WARN_ON and panic the kernel via an invalid DMA address. -- --- Patch [1]: [PATCH] accel/rocket: Validate task regcmd address and count on submission --- commit f9e96d89fbac295861e6b6ae4bd9fc056fddf28a Author: Sidong Yang <[email protected]> Subject: accel/rocket: Validate task regcmd address and count on submission This commit adds validation to the drm_rocket_task regcmd fields from userspace. It rejects unaligned addresses and out-of-range counts with -EINVAL at submission time to prevent hardware drops and mode flips. > 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; > } > > + 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; [Severity: Critical] This is a pre-existing issue, but can the loop counter i overflow here? Looking at rocket_copy_tasks(), the loop is defined as: for (int i = 0; i < rjob->task_count; i++) Since rjob->task_count is a u32 controlled by userspace and i is a signed 32-bit integer, if a user requests exactly 0x80000001 tasks, could i wrap to a negative value? If i wraps to a negative value, does this assignment allow an out-of-bounds memory write before the allocated array? > rjob->tasks[i].regcmd_count = task.regcmd_count; > } [ ... ] [Severity: High] This isn't a bug introduced by this patch, but while reviewing the error handling for these new -EINVAL checks, it appears the return value of rocket_ioctl_submit_job() might be ignored. drivers/accel/rocket/rocket_job.c:rocket_ioctl_submit() { ... rocket_ioctl_submit_job(dev, file, &jobs[i]); exit: kvfree(jobs); return ret; } Does this allow the ioctl to silently return 0 even if the job was rejected by the validations added in this patch? [Severity: High] This is a pre-existing issue, but does this code leak an IOMMU group reference on every job completion? drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() { ... iommu_detach_group(NULL, iommu_group_get(core->dev)); ... } Since iommu_group_get() increments the kref and iommu_detach_group() does not consume it, are we missing a matching iommu_group_put()? [Severity: High] This is a pre-existing issue, but can the 32-bit regcmd truncate 64-bit IOVAs? include/uapi/drm/rocket_accel.h:drm_rocket_task { ... __u32 regcmd; ... } The allocator appears to be given the full IOMMU aperture: drivers/accel/rocket/rocket_drv.c:rocket_open() { ... drm_mm_init(&rocket_priv->mm, start, end - start + 1); ... } If a buffer object is allocated above 4GB, does the UAPI structure truncate the DMA address, causing the NPU to read unmapped memory? [Severity: High] This is a pre-existing issue, but can unprivileged userspace trigger a kernel panic here? drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() { ... WARN_ON(raw_status & PC_INTERRUPT_RAW_STATUS_DMA_READ_ERROR); ... } If userspace provides an invalid DMA address, the hardware will trigger a DMA read error. Could this WARN_ON crash the system if panic_on_warn is enabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
