From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm: Add PERFCNTR_CONFIG ioctl
Date: Sat, 16 May 2026 15:20:25 +1000
Message-ID: <review-patch13-20260511130017.96867-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260511130017.96867-14-robin.clark@oss.qualcomm.com>
References: <20260511130017.96867-1-robin.clark@oss.qualcomm.com> <20260511130017.96867-14-robin.clark@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the core ioctl patch. Several issues:

**(1) CRITICAL: Stack buffer overflow via `group_stride`.**

```c
struct drm_msm_perfcntr_group g = {0};  // 32 bytes on stack
...
if (copy_from_user(&g, userptr, args->group_stride))
```

`args->group_stride` is only validated to be non-zero. If userspace passes `group_stride = 256`, this writes 256 bytes into a 32-byte stack variable. Similarly, the `copy_to_user` in the `MSM_PERFCNTR_UPDATE` path would leak stack contents to userspace.

Fix: cap the copy size, e.g.:
```c
size_t copy_sz = min_t(size_t, args->group_stride,
                       sizeof(struct drm_msm_perfcntr_group));
if (copy_from_user(&g, userptr, copy_sz))
```

**(2) Undefined behavior in `1 << bufsz_shift`.**

```c
stream->fifo_size = 1 << args->bufsz_shift;
```

`1` is `int` (32-bit). If `bufsz_shift >= 31`, this is signed integer overflow (UB). And `fifo_size` is `size_t`, so the assignment would also be wrong on 64-bit. Later:

```c
void *buf __free(kfree) =
    kmalloc(1 << args->bufsz_shift, GFP_KERNEL);
```

Same UB repeated. Fix: use `1UL << args->bufsz_shift` and validate `bufsz_shift` early (e.g., `bufsz_shift > 27` returns EINVAL, consistent with the 128M limit).

**(3) Wrong variable in error message:**

```c
if (stream->fifo_size > SZ_128M)
    return UERR(EINVAL, dev, "buffer size too big (>128M): %zu", bufsz);
```

Should print `stream->fifo_size`, not `bufsz` (which holds the per-period size at that point).

**(4) Error path leaks allocated counters.** If the ioctl returns an error after writing to `perfcntrs->groups[idx]->allocated_counters` (which happens in the loop at line 7432), those counters remain "allocated" even though no stream was created. The error paths before `if (ret) return ret;` should reset `allocated_counters` to 0 for all groups.

**(5) UAPI note:** The ioctl is `DRM_IOW` (write-only from user to kernel). The comment in the UAPI header explains this is intentional since the fd is returned via ioctl return value. This is correct and well-documented.

**(6) Missing `min_t` for group_stride validation would also fix forward compatibility.** If a future kernel adds fields to `drm_msm_perfcntr_group`, old userspace with a smaller struct would work correctly with `min()`. Conversely, new userspace with a bigger struct against this kernel would silently lose extra fields. This is the standard extensible-struct pattern.

---
Generated by Claude Code Patch Reviewer