Hi Michael,
Thanks for your patch!
On 17/06/26 09:58, Michael Bommarito wrote:
+
+/* Reject a query CPU job whose writes would land outside their BO. */
+static int
+v3d_cpu_job_bounds_check(struct v3d_cpu_job *job)
+{
+ struct drm_device *dev = &job->base.v3d->drm;
+ struct v3d_timestamp_query_info *tquery = &job->timestamp_query;
+ struct v3d_copy_query_results_info *copy = &job->copy;
+ u32 elem = copy->do_64bit ? sizeof(u64) : sizeof(u32);
+ struct v3d_bo *dst, *src;
+ u64 slots, write_size;
+ int i;
Sashiko pointed out that this should be a u32 at least.
+
+ switch (job->job_type) {
+ case V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY:
+ case V3D_CPU_JOB_TYPE_RESET_TIMESTAMP_QUERY:
+ /*
+ * Each query writes one u64 timestamp slot into bo[0]. The
+ * offset is cast to u64 so a 0xffffffff query offset cannot
wrap
+ * the addition past the BO size.
+ */
"/* Each query writes one u64 timestamp slot into bo[0]. */" was enough
of a comment.
+ dst = to_v3d_bo(job->base.bo[0]);
+
+ for (i = 0; i < tquery->count; i++) {
+ if ((u64)tquery->queries[i].offset + sizeof(u64) >
+ dst->base.base.size)
+ goto err_range;
+ }
+ return 0;
+ case V3D_CPU_JOB_TYPE_COPY_TIMESTAMP_QUERY:
+ /* Copies one u64 per query from bo[1] into bo[0]. */
+ dst = to_v3d_bo(job->base.bo[0]);
+ src = to_v3d_bo(job->base.bo[1]);
+
+ for (i = 0; i < tquery->count; i++) {
+ if ((u64)tquery->queries[i].offset + sizeof(u64) >
+ src->base.base.size)
+ goto err_range;
+ }
+
+ write_size = (copy->availability_bit ? 2 : 1) * elem;
+ return v3d_check_copy_extent(dev, dst->base.base.size,
+ copy->offset, copy->stride,
+ tquery->count, write_size);
+ case V3D_CPU_JOB_TYPE_COPY_PERFORMANCE_QUERY:
+ /*
+ * Each query writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
+ * counter slots into bo[0], plus an availability slot at index
+ * ncounters. nperfmons and ncounters are user values; the slot
+ * count and write size are computed in u64 (u32 * small
+ * constant) so they cannot wrap.
+ */
I preferred the previous comment (from v2):
/*
* Each query writes nperfmons * DRM_V3D_MAX_PERF_COUNTERS
* counter slots into bo[0], plus an availability slot at
* index ncounters. nperfmons and ncounters are user values,
* so the slot count is computed overflow-safe.
*/
+ dst = to_v3d_bo(job->base.bo[0]);
+
+ slots = (u64)job->performance_query.nperfmons *
+ DRM_V3D_MAX_PERF_COUNTERS;
+ if (copy->availability_bit)
+ slots = max(slots,
+ (u64)job->performance_query.ncounters + 1);
+
+ write_size = slots * elem;
+ return v3d_check_copy_extent(dev, dst->base.base.size,
+ copy->offset, copy->stride,
+ job->performance_query.count,
+ write_size);
+ case V3D_CPU_JOB_TYPE_INDIRECT_CSD: {
+ struct v3d_indirect_csd_info *icsd = &job->indirect_csd;
I'd prefer to call it "indirect_csd".
+
+ /*
+ * The exec-time work-group rewrite reads three u32 counts from
+ * bo[0] at the user-supplied offset, then writes each count
back
+ * into the indirect BO at a user-supplied u32 index. Reject a
+ * missing indirect BO (drm_gem_object_lookup() returned NULL
for
+ * a bad handle, which the exec path would otherwise
dereference)
+ * and bound both accesses.
+ */
+ if (!icsd->indirect)
+ return -ENOENT;
No need for that after [1].
[1]
https://lore.kernel.org/dri-devel/[email protected]/T/#m6c2cb9021bc6b524fb713150f94bdfee3553cf72
+
+ dst = to_v3d_bo(job->base.bo[0]);
New line. Also, instead of the big comment on the beginning of this
case, I'd prefer a smaller comment here explaining that 3 is the three
dimensions of workgroup counts (x,y,z).
+ if ((u64)icsd->offset + 3 * sizeof(u32) > dst->base.base.size)
+ goto err_range;
+
+ src = to_v3d_bo(icsd->indirect);
I'm not sure src makes a lot of sense here, as we copy from BO to the
indirect BO. Maybe dst?
+ for (i = 0; i < 3; i++) {
+ u32 uidx = icsd->wg_uniform_offsets[i];
+
+ if (uidx != 0xffffffff &&
Add a comment explaining that, as 0xffffffff means "skip this rewrite",
the exec path doesn't write it, so there is no need to range-check it.
Finally, although it's a fix, I'd prefer to apply it to drm-misc-next as
(1) it's a reasonably big change and (2) it's not a critical bug. So, if
possible, rebase the patch on top of drm-misc-next.
Best regards,
- Maíra
