[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Koenig, Christian <[email protected]> > Sent: Tuesday, March 17, 2026 3:08 PM > To: Zhang, Jesse(Jie) <[email protected]>; [email protected] > Cc: Deucher, Alexander <[email protected]>; Prosyak, Vitaly > <[email protected]> > Subject: Re: [PATCH 3/3] drm/amdgpu: harden SIGNAL/WAIT ioctl argument > validation > > On 3/17/26 02:17, Jesse.Zhang wrote: > > Tighten the early parameter checks in the USERQ SIGNAL and WAIT ioctls: > > > > 1. Validate num_syncobj_handles against AMDGPU_USERQ_MAX_HANDLES > in > > addition to the BO handle counts that are already checked. The UAPI > > field is __u64 but the driver stores it in a u32, so the comparison > > must happen before the narrowing assignment to prevent unintended > > truncation (e.g. 0x1_0000_0000 would silently become 0). > > > > 2. Reject inconsistent pointer/count pairs where a non-NULL userspace > > pointer is provided with a zero element count. This is clearly > > malformed input and returning -EINVAL early gives userspace a > > deterministic error rather than silently proceeding with empty data. > > > > No functional change for well-formed userspace callers. > > > > v2: > > - Reworked commit message to focus on parameter validation correctness > > - Updated code comments for clarity: describe the type width mismatch > > and why the early check is needed > > - No functional changes to the code itself > > > > Signed-off-by: Jesse Zhang <[email protected]> > > Reviewed-by: Vitaly Prosyak <[email protected]> > > --- > > .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 37 ++++++++++++++++++- > > 1 file changed, 35 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > > index fad595401a77..575dd58ed152 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > > @@ -480,8 +480,25 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, > void *data, > > if (!amdgpu_userq_enabled(dev)) > > return -ENOTSUPP; > > > > + /* > > + * num_syncobj_handles is __u64 in the UAPI but stored in a u32 > > + * in the driver. Check all three handle counts against the > > + * maximum *before* the narrowing assignment so that values > > + * above 2^32 are correctly rejected instead of being silently > > + * truncated to a smaller (possibly zero) value. > > + */ > > That is completely unnecessary, just reduce the variables size in the UAPI as > we > already did for num_syncobj_handles. > > > > > if (args->num_bo_write_handles > AMDGPU_USERQ_MAX_HANDLES || > > - args->num_bo_read_handles > AMDGPU_USERQ_MAX_HANDLES) > > + args->num_bo_read_handles > AMDGPU_USERQ_MAX_HANDLES || > > + args->num_syncobj_handles > AMDGPU_USERQ_MAX_HANDLES) { > > You don't need to check num_syncobj_handles here. Thanks Christian. I will drop the num_syncobj_handles > AMDGPU_USERQ_MAX_HANDLES checks from both SIGNAL and WAIT paths.
Only keep the pointer/count consistency checks (non-NULL pointer with zero count -> -EINVAL), since those are still useful for deterministic early input validation and do not change behavior for valid userspace. Thanks Jesse > > Regards, > Christian. > > > + return -EINVAL; > > + } > > + > > + /* Reject non-NULL pointers paired with a zero count. */ > > + if (!args->num_syncobj_handles && args->syncobj_handles) > > + return -EINVAL; > > + if (!args->num_bo_read_handles && args->bo_read_handles) > > + return -EINVAL; > > + if (!args->num_bo_write_handles && args->bo_write_handles) > > return -EINVAL; > > > > num_syncobj_handles = args->num_syncobj_handles; @@ -639,7 +656,23 > > @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data, > > return -ENOTSUPP; > > > > if (wait_info->num_bo_write_handles > > AMDGPU_USERQ_MAX_HANDLES || > > - wait_info->num_bo_read_handles > > AMDGPU_USERQ_MAX_HANDLES) > > + wait_info->num_bo_read_handles > > AMDGPU_USERQ_MAX_HANDLES || > > + wait_info->num_syncobj_handles > > AMDGPU_USERQ_MAX_HANDLES) > > + return -EINVAL; > > + > > + /* Reject non-NULL pointers paired with a zero count: the pointer > > + * is meaningless and indicates inconsistent input from userspace. > > + */ > > + if (!wait_info->num_syncobj_handles && wait_info->syncobj_handles) > > + return -EINVAL; > > + if (!wait_info->num_syncobj_timeline_handles && > > + (wait_info->syncobj_timeline_handles || wait_info- > >syncobj_timeline_points)) > > + return -EINVAL; > > + if (!wait_info->num_bo_read_handles && wait_info->bo_read_handles) > > + return -EINVAL; > > + if (!wait_info->num_bo_write_handles && wait_info->bo_write_handles) > > + return -EINVAL; > > + if (!wait_info->num_fences && wait_info->out_fences) > > return -EINVAL; > > > > num_syncobj = wait_info->num_syncobj_handles;
