Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [Critical] Double free vulnerability in job submission error path - [Critical] NULL pointer dereference when tearing down a failed job initialization - [High] Integer overflow in `drm_gem_lock_reservations` causes complete locking bypass - [High] Aborting an armed DRM scheduler job violates API contract and corrupts sequence numbers - [High] Missing `DMA_RESV_USAGE_READ` fence attachment leads to Write-After-Read data corruption - [High] Memory leak of `scheds` array on success path for single-core devices -- --- Patch [1]: [PATCH v5 1/2] drm/rocket: Check allocations before use --- commit 597ae023ed9125965a9c68230305382ffec6ab1d Author: Triet Hoang <[email protected]> drm/rocket: Check allocations before use Check the result of kvmalloc_array() in rocket_job_push() and kmalloc_objs() in rocket_job_open() before using the allocated buffers. > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index ac51bff39833f..c4a40fe6ab973 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -189,9 +189,15 @@ static int rocket_job_push(struct rocket_job *job) > struct drm_gem_object **bos; > struct ww_acquire_ctx acquire_ctx; > int ret = 0; > + size_t bos_count; > + > + if (check_add_overflow(job->in_bo_count, job->out_bo_count, &bos_count)) > + return -EOVERFLOW; > + > + bos = kvmalloc_array(bos_count, sizeof(void *), GFP_KERNEL); > + if (!bos) > + return -ENOMEM; > > - bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void > *), > - GFP_KERNEL); > memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); > memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * > sizeof(void *)); [Severity: Critical] This is a pre-existing issue, but is there a potential double free during the job submission error path? If rocket_copy_tasks() fails (for example, due to an invalid copy_from_user()), it calls kvfree(rjob->tasks) but does not set the pointer to NULL: rocket_copy_tasks() kvfree(rjob->tasks); // rjob->tasks is left dangling When the error propagates up to rocket_ioctl_submit_job(), it calls rocket_job_put(rjob) which triggers the cleanup routine. The cleanup function unconditionally frees the tasks array a second time: rocket_job_cleanup() kvfree(job->tasks); Could this double-free be triggered from unprivileged user space? [Severity: Critical] This is also a pre-existing issue, but could we hit a NULL pointer dereference when tearing down a failed job initialization? In rocket_ioctl_submit_job(), rjob is allocated with kzalloc_obj(), which leaves rjob->domain initialized to NULL. If an error occurs before the IOMMU domain is fetched (for example, if drm_sched_job_init() fails), the error path invokes rocket_job_cleanup(). The cleanup function unconditionally calls rocket_iommu_domain_put() on the domain pointer: rocket_iommu_domain_put() kref_put(&domain->kref, rocket_iommu_domain_destroy); Since domain is NULL, would this immediate dereference cause a kernel panic reachable by userspace? > ret = drm_gem_lock_reservations(bos, job->in_bo_count + > job->out_bo_count, &acquire_ctx); [Severity: High] This isn't a bug introduced by this patch, but does the sum of the buffers overflow when passed to drm_gem_lock_reservations()? While this patch adds check_add_overflow() to prevent size_t overflow during the array allocation, the unchecked sum is still passed directly to drm_gem_lock_reservations(). Since drm_gem_lock_reservations() accepts a signed int for the count, could providing individually valid but large arrays (e.g., 0x40000000 each) result in a sum of 0x80000000? If this implicitly casts to a negative integer (-2147483648), it appears drm_gem_lock_reservations() would skip its loop and return success immediately. Would this bypass the GEM locking mechanism and lead to data races? > if (ret) > goto err; > > scoped_guard(mutex, &rdev->sched_lock) { > drm_sched_job_arm(&job->base); [Severity: High] This is a pre-existing issue, but does this error path violate the DRM scheduler API contract? In rocket_job_push(), the job is armed, establishing a point of no return. Subsequently, the driver attempts to acquire object fences. If this fails, the function returns an error: > > job->inference_done_fence = > dma_fence_get(&job->base.s_fence->finished); > > ret = rocket_acquire_object_fences(job->in_bos, > job->in_bo_count, &job->base, false); > if (ret) > goto err_unlock; The DRM scheduler documentation explicitly forbids aborting a job once drm_sched_job_arm() is called; it must be pushed. Does aborting the job here orphan the sequence number and corrupt the scheduler state? [ ... ] > rocket_attach_object_fences(job->out_bos, job->out_bo_count, > job->inference_done_fence); [Severity: High] This is a pre-existing issue, but is the completion fence missing for the input buffers? After the job is pushed, rocket_attach_object_fences() attaches the completion fence to out_bos. However, it doesn't appear to attach the fence to the input buffers (in_bos) with DMA_RESV_USAGE_READ. Without this attachment, does the DRM reservation system know the hardware is reading from these buffers? Could a subsequent operation write to these buffers without waiting, resulting in a write-after-read data corruption? [ ... ] > @@ -501,6 +507,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > unsigned int core; > int ret; > > + if (!scheds) > + return -ENOMEM; > + > for (core = 0; core < rdev->num_cores; core++) > scheds[core] = &rdev->cores[core].sched; > > @@ -509,9 +518,9 @@ int rocket_job_open(struct rocket_file_priv *rocket_priv) > scheds, > rdev->num_cores, NULL); > if (WARN_ON(ret)) > - return ret; > + kfree(scheds); > > - return 0; > + return ret; [Severity: High] This is a pre-existing issue, but is there a memory leak of the scheds array on the success path for single-core devices? When drm_sched_entity_init() is called with num_sched_list equal to rdev->num_cores, and num_cores is 1, the DRM core sets entity->sched_list to NULL internally. Because rocket_job_open() returns without freeing scheds or storing the pointer locally when entity->sched_list is internally nulled, is this dynamically allocated array leaked? Could a malicious userspace program exhaust kernel memory by repeatedly opening and closing the device node? > } > > void rocket_job_close(struct rocket_file_priv *rocket_priv) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
