From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/amdkfd: fix integer overflow in get_queue_ids() Date: Mon, 25 May 2026 17:31:38 +1000 Message-ID: In-Reply-To: <20260523142645.39102-1-meatuni001@gmail.com> References: <20260523165646.25645-1-meatuni001@gmail.com> <20260523142645.39102-1-meatuni001@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good** The fix changes: ```c if (!usr_queue_id_array) return NULL; ``` to: ```c if (!usr_queue_id_array) return num_queues ? ERR_PTR(-EINVAL) : NULL; ``` This addresses a real bug: in `suspend_queues()` (line 3603), `get_queue_id= s()` is called unconditionally. If userspace supplies `num_queues > 0` with= `queue_array_ptr =3D=3D 0`, `get_queue_ids()` previously returned NULL. Si= nce `IS_ERR(NULL) =3D=3D false`, execution continued to `q_array_invalidate= (num_queues, NULL)` at line 3611, which dereferences NULL in a loop =E2=80= =94 kernel panic. The `num_queues =3D=3D 0` case correctly still returns NULL, preserving the= existing no-op behavior. In `suspend_queues()`, this means `q_array_invali= date(0, NULL)` is called but doesn't iterate (loop bound is 0), the `q_arra= y_get_index` calls also use `num_queues =3D=3D 0` so they don't iterate, an= d `copy_to_user(NULL, NULL, 0)` is a no-op. `kfree(NULL)` is also a no-op. = So the NULL return path in `suspend_queues` is safe, though it's a pre-exis= ting asymmetry with `resume_queues` which guards the tail behind `if (queue= _ids)`. In `resume_queues()` (line 3500), there's already a `if (usr_queue_id_array= )` guard before calling `get_queue_ids()`, so the NULL dereference path was= only exploitable via `suspend_queues()`. The fix is still correct for both= callers. **Minor observation (pre-existing, not introduced by this patch)**: `suspen= d_queues()` at lines 3704-3708 does not guard `copy_to_user()` and `kfree()= ` with a NULL check on `queue_ids`, unlike `resume_queues()` at line 3586. = When `get_queue_ids()` returns NULL (the `num_queues =3D=3D 0` no-op case),= `copy_to_user(NULL, NULL, 0)` happens to be benign on all current architec= tures, but wrapping those calls in `if (queue_ids)` for consistency with `r= esume_queues` would be a small improvement. Not a bug, not a blocker, and o= ut of scope for this fix. **No issues found with the patch itself.** --- Generated by Claude Code Patch Reviewer