Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] drm/msm/gem: invalid pointer deref in msm_vma_job_free() on lookup failure --
commit 0d94fd96560aa90ec37575d957f23c1d0f49e11c Author: Rob Clark <[email protected]> drm/msm/gem: Decrement loop idx if handle lookup fails This commit decrements the loop index i when idr_find() fails in vm_bind_job_lookup_ops() so that the out_unlock cleanup loop skips the entry that still holds a GEM handle rather than a GEM object pointer. > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c > b/drivers/gpu/drm/msm/msm_gem_vma.c > index 922285537dee0..4413db0644a29 100644 > --- a/drivers/gpu/drm/msm/msm_gem_vma.c > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c > @@ -1110,6 +1110,7 @@ vm_bind_job_lookup_ops(struct msm_vm_bind_job *job, > struct drm_msm_vm_bind *args > obj = idr_find(&file->object_idr, op->handle); > if (!obj) { > ret = UERR(EINVAL, dev, "invalid handle %u at index > %u\n", op->handle, i); > + i--; > goto out_unlock; > } [Severity: Critical] Does skipping the failed op here without resetting job->nr_ops = 0 in the out_unlock error path leave un-looked-up GEM handles in job->ops[] for msm_vma_job_free() to dereference? In struct msm_vm_bind_op, obj and handle share an anonymous union: struct msm_vm_bind_op { uint32_t op; uint32_t flags; union { struct drm_gem_object *obj; uint32_t handle; }; ... } ops[]; During the first loop in vm_bind_job_lookup_ops(), lookup_op() increments job->nr_ops and stores each operation's handle in job->ops[i].handle. When idr_find() fails (or when the subsequent size_add() range check fails before remaining ops are converted), the out_unlock error block only zeroes op->obj for indices <= i and does not reset job->nr_ops = 0: out_unlock: spin_unlock(&file->table_lock); if (ret) { for (; i >= 0; i--) { struct msm_vm_bind_op *op = &job->ops[i]; if (!op->obj) continue; drm_gem_object_put(op->obj); op->obj = NULL; } } When vm_bind_job_lookup_ops() returns an error to msm_ioctl_vm_bind(), it calls msm_vma_job_free(&job->base): msm_ioctl_vm_bind() -> msm_vma_job_free() -> job_foreach_bo(obj, job) drm_gem_object_put(obj); Because job_foreach_bo() iterates up to job->nr_ops, won't drm_gem_object_put() be called on the raw 32-bit GEM handle left in the union for the failed op (and any subsequent ops in the array)? Should the if (ret) block at out_unlock also set job->nr_ops = 0, just like the earlier lookup_op() error path does? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
