Re: [PATCH v3 2/3] drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds checking of user-supplied offsets leads to arbitrary kernel memory read and write in CPU job execution functions.
- [Critical] Double free of `query_info->queries` arrays on error paths when processing timestamp or performance query extensions.
- [High] Missing return value check for `vmap()` failure in `v3d_get_bo_vaddr()` leads to NULL pointer dereferences.
- [High] Memory leak of `se.out_syncs` in non-CPU IOCTLs when `v3d_get_extensions` fails.
--

commit 3cd6d13669aae5071cfb72bf592569413ffc98ac
Author: Maíra Canal <mcanal@igalia.com>

drm/v3d: Reject invalid indirect BO handle in indirect CSD setup

This commit fixes a NULL pointer dereference in the v3d driver caused by
failing to check the return value of drm_gem_object_lookup(). It updates
v3d_get_cpu_indirect_csd_params() to bail out with -ENOENT if the userspace
supplied indirect BO handle is invalid.

> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index 9d23935b05dd0..656b0bbd4f8e3 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -601,6 +601,8 @@ v3d_get_cpu_indirect_csd_params(struct drm_file *file_priv,
>  	       sizeof(indirect_csd.wg_uniform_offsets));
>  
>  	info->indirect = drm_gem_object_lookup(file_priv, indirect_csd.indirect);
> +	if (!info->indirect)
> +		return -ENOENT;
>  
>  	return 0;
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing the CPU job
code, does this lack bounds checking on the user-supplied offsets?

In drivers/gpu/drm/v3d/v3d_sched.c: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];

And in v3d_timestamp_query():
    value_addr = ((u8 *)bo->vaddr) + timestamp_query->queries[i].offset;
    *((u64 *)value_addr) = i == 0 ? ktime_get_ns() : 0ull;

When a user submits a CPU job (like indirect CSD or timestamp query), can
they pass arbitrary offset values? 

Since these offsets are added to the vmap'd kernel addresses of the BOs
during job execution in the scheduler thread, could this allow arbitrary
read and write access to kernel memory?

[Severity: High]
This is a pre-existing issue, but does this code fail to handle vmap()
returning NULL?

In drivers/gpu/drm/v3d/v3d_bo.c:v3d_get_bo_vaddr():
    bo->vaddr = vmap(obj->pages, obj->base.size >> PAGE_SHIFT, VM_MAP, 
                     pgprot_writecombine(PAGE_KERNEL));

If vmap() fails due to memory pressure, bo->vaddr is set to NULL but no
error is returned.

Callers then unconditionally use the mapped address assuming success:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_rewrite_csd_job_wg_counts_from_indirect() {
    ...
    v3d_get_bo_vaddr(bo);
    wg_counts = (uint32_t *)(bo->vaddr + indirect_csd->offset);
    ...
}

Could this trigger a NULL pointer dereference when vmalloc space is
exhausted?

[Severity: High]
This isn't a bug introduced by this patch, but does this early return leak
se.out_syncs?

In drivers/gpu/drm/v3d/v3d_submit.c, non-CPU IOCTLs handle extension parsing
like this:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_submit_cl_ioctl() {
    ...
    ret = v3d_get_extensions(file_priv, args->extensions, &se, NULL);
    if (ret) {
        drm_dbg(dev, "Failed to get extensions.\n");
        return ret;
    }
    ...
}

If a user provides a valid DRM_V3D_EXT_ID_MULTI_SYNC extension followed by
an invalid one in the linked list, the MULTI_SYNC handler successfully
allocates se->out_syncs.

When parsing the invalid extension fails, v3d_get_extensions() returns an
error. By returning directly instead of jumping to the fail_submit cleanup
label, do we leak the struct v3d_submit_outsync allocations?

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a double free?

When v3d_get_cpu_timestamp_query_params() encounters an error (like an
invalid syncobj), it calls v3d_timestamp_query_info_free():

drivers/gpu/drm/v3d/v3d_submit.c:v3d_get_cpu_timestamp_query_params() {
    ...
error:
    v3d_timestamp_query_info_free(&job->timestamp_query, i);
    return err;
}

This frees the array but leaves the pointer intact:

drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query_info_free() {
    ...
    kvfree(query_info->queries);
}

The IOCTL then propagates the error and jumps to cleanup, which unconditionally
destroys the job:

drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_free() {
    ...
    v3d_timestamp_query_info_free(&job->timestamp_query,
                                  job->timestamp_query.count);
}

Since count remains 0, the inner loop is skipped, but does kvfree() get
called again on the dangling query_info->queries pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-v3d-cpu-job-fixes-v3-0-bc51b1f3eeb5@igalia.com?part=2