Re: [PATCH] drm/amdkfd: reject alloc_memory_of_gpu with wrapping va_addr/mmap_offset

On Thu, Jul 30, 2026 at 12:03=E2=80=AFPM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s) to consider:
> - [High] The patch adds an unconditional overflow check on `args->mmap_of=
fset`, which is strictly an OUT parameter for most allocation types. This w=
ill cause perfectly valid ioctl calls to randomly fail with `-EINVAL` if us=
erspace leaves the output field uninitialized.
> --
>
> --- Patch [1]: [PATCH] drm/amdkfd: reject alloc_memory_of_gpu with wrappi=
ng va_addr/mmap_offset ---
> commit 90e63e5cacd60faaf13c89e037955751ad085487
> Author: Hari Mishal <[email protected]>
>
> drm/amdkfd: reject alloc_memory_of_gpu with wrapping va_addr/mmap_offset
>
> This commit adds check_add_overflow() checks to kfd_ioctl_alloc_memory_of=
_gpu()
> to prevent integer wrap-arounds when calculating the end address of va_ad=
dr
> and mmap_offset.
>
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm=
/amd/amdkfd/kfd_chardev.c
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > @@ -1089,10 +1089,15 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct=
 file *filep,
> [ ... ]
> >       if (args->size =3D=3D 0)
> >               return -EINVAL;
> >
> > +     if (check_add_overflow(args->va_addr, args->size, &end) ||
> > +         check_add_overflow(args->mmap_offset, args->size, &end))
> > +             return -EINVAL;
> > +
>
> [Severity: High]
> Will this check cause valid allocation requests to fail if the user space
> application leaves args->mmap_offset uninitialized?
>
> According to the uAPI in kfd_ioctl.h, mmap_offset is an OUT parameter
> for non-USERPTR allocations:
>
> struct kfd_ioctl_alloc_memory_of_gpu_args {
>         ...
>         __u64 mmap_offset;      /* to KFD (userptr), from KFD (mmap offse=
t) */
>         ...
> };
>
> Since this check is unconditional, if userspace calls this ioctl without =
the
> KFD_IOC_ALLOC_MEM_FLAGS_USERPTR flag and leaves the mmap_offset field
> uninitialized on the stack, random garbage in that field could trigger th=
is
> overflow check and spuriously return -EINVAL.
>
> Should the args->mmap_offset overflow check be restricted to only run whe=
n
> (args->flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) is true?

Hi Hari,

I think this change makes sense.  Can you take a look and respin?

Thanks,

Alex