Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Logical Use-After-Free/Double-Free introduced by attempting to manually free a job already tracked in the global submit array. - [Low] The patch introduces calls to an undefined function `v3d_job_deallocate()`, which will cause a build failure. Pre-existing issues: - [Critical] Pre-existing double-free vulnerability in the extension parsing error paths for CPU jobs. -- --- Patch [1]: [PATCH] drm/v3d: Fix NULL pointer dereference in CPU job cleanup --- commit f37328e13f9e40584da544c634e4094ecc1b59ee Author: Karl Mehltretter <[email protected]> drm/v3d: Fix NULL pointer dereference in CPU job cleanup This commit attempts to fix a NULL pointer dereference in CPU job cleanup by manually deallocating the job and clearing the pointer when extension parsing or BO count validation fails. It aims to match the error handling present when v3d_job_init() fails. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 834d52030979d..8cabb3ba77898 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c > @@ -1573,6 +1573,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data, > ret = v3d_get_extensions(file_priv, args->extensions, &se, > cpu_job); > if (ret) { > drm_dbg(dev, "Failed to get extensions.\n"); > + v3d_job_deallocate((void *)&cpu_job); [Severity: Low] Does this code break the build? It appears v3d_job_deallocate() is not defined or declared anywhere in the kernel tree. [Severity: High] If v3d_job_deallocate() were defined and freed the job, would this cause a double-free? The cpu_job was already added to the global submit array earlier in v3d_submit_cpu_ioctl() via: cpu_job = (struct v3d_cpu_job *)v3d_submit_add_job(&submit, V3D_CPU); When this code jumps to the fail label, it executes v3d_submit_cleanup_jobs(), which iterates over the submit.jobs array and calls v3d_job_cleanup() on the pointer. If the job was already manually freed here, would v3d_submit_cleanup_jobs() dereference a dangling pointer? [Severity: Critical] This is a pre-existing issue, but does v3d_get_extensions() leave a dangling pointer that gets double-freed on this error path? If a user provides an invalid sync handle, v3d_get_cpu_timestamp_query_params() fails and calls v3d_timestamp_query_info_free(): drivers/gpu/drm/v3d/v3d_sched.c:v3d_timestamp_query_info_free() { ... kvfree(query_info->queries); ... } The queries array is freed but the pointer is not set to NULL. The error bubbles up here, and the code jumps to the fail label. The fail path calls v3d_submit_cleanup_jobs(), which calls v3d_cpu_job_free() on the same job, and that unconditionally calls v3d_timestamp_query_info_free() again: drivers/gpu/drm/v3d/v3d_submit.c:v3d_cpu_job_free() { ... v3d_timestamp_query_info_free(&job->timestamp_query, job->timestamp_query.count); ... } Will this result in a double-free of query_info->queries? > goto fail; > } > } [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
