Re: [PATCH v2] drm/v3d: bound CPU-job query writes to their destination BO

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The V3D_CPU_JOB_TYPE_INDIRECT_CSD job type was omitted from the new CPU job bounds checks, leaving unmitigated arbitrary out-of-bounds kernel memory read and write vulnerabilities.
- [High] v3d_get_cpu_indirect_csd_params does not check if drm_gem_object_lookup returns NULL for the indirect handle, leading to a NULL pointer dereference during job execution.
- [High] Kernel crash due to calling cleanup functions on uninitialized memory if CPU job validation fails early.
--

commit 0ce9f49e205ba6d1094ea08101223cdd03baade5
Author: Michael Bommarito <michael.bommarito@gmail.com>
Subject: drm/v3d: bound CPU-job query writes to their destination BO

This commit adds bounds checking for CPU job queries to ensure they do not
write past their destination buffer object. It validates the full write
extent against the destination BO size before the job is queued.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 1db43c6a078d5..ed45989b84b28 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
[ ... ]
> +/* Reject a query CPU job whose writes would land outside their BO. */
> +static int
> +v3d_cpu_job_bounds_check(struct v3d_cpu_job *job)
> +{
[ ... ]
> +	switch (job->job_type) {
> +	case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY:
> +	case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY:
[ ... ]
> +	default:
> +		return 0;

[Severity: Critical]
This isn't a bug introduced by this patch, but since we are adding bounds
checking here, should V3D_CPU_JOB_TYPE_INDIRECT_CSD be validated as well?

Because this switch falls through to the default case for INDIRECT_CSD
jobs, the user-controlled indirect_csd->offset is left unvalidated.
When this is later used in v3d_rewrite_csd_job_wg_counts_from_indirect():

v3d_rewrite_csd_job_wg_counts_from_indirect()
    wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
    ...
    ((uint32_t *)indirect->vaddr)[uniform_idx] = wg_counts[i];

Could this allow out-of-bounds kernel memory reads and writes?

> +	}
> +
> +err_range:
> +	drm_dbg(dev, "CPU job query offset exceeds the BO.\n");
> +	return -EINVAL;
> +}
> +
>  /**
>   * v3d_submit_cpu_ioctl() - Submits a CPU job to the V3D.
>   * @dev: DRM device
> @@ -1346,6 +1444,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
>  		if (ret)
>  			goto fail;
>  
> +		ret = v3d_cpu_job_bounds_check(cpu_job);
> +		if (ret)
> +			goto fail;
> +

[Severity: High]
This is a pre-existing issue, but does calling goto fail here or earlier
in v3d_submit_cpu_ioctl() risk a NULL pointer dereference?

If validation fails before v3d_job_init() is called (for example, if
bo_handle_count is incorrect), cpu_job->base.s_fence remains NULL. The
error path then cleans up the uninitialized job:

v3d_submit_cpu_ioctl()
    ...
    if (args->bo_handle_count != ...)
        goto fail;
    ...
fail:
    v3d_job_cleanup((void *)cpu_job);

When drm_sched_job_cleanup() is reached:

drm_sched_job_cleanup()
    if (kref_read(&job->s_fence->finished.refcount)) {

Would this unconditionally dereference the NULL s_fence and crash the
kernel?

[Severity: High]
This is another pre-existing issue in the CPU job submission path, but
is it possible for the indirect_csd parameters to trigger a crash if
the user provides an invalid GEM handle?

In v3d_get_cpu_indirect_csd_params(), the return value of
drm_gem_object_lookup() is stored without a NULL check:

v3d_get_cpu_indirect_csd_params()
    info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);

If this is NULL, when the job executes:

v3d_rewrite_csd_job_wg_counts_from_indirect()
    v3d_get_bo_vaddr(indirect);

v3d_get_bo_vaddr()
    bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, ...);

Will this lead to an immediate NULL pointer dereference when obj->pages or
obj->base.size is accessed?

>  		ret = v3d_lock_bo_reservations(&cpu_job->base, &acquire_ctx);
>  		if (ret)
>  			goto fail;

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