[PATCH] drm/amdgpu: validate the parameters of amdgpu_vm_bo_clear_mappings

[Vlad Stolyarov]

+Jann Horn <jannh at google.com> for his thoughts

On Thu, Apr 11, 2024 at 12:25 PM Christian König <christian.koenig at amd.com>
wrote:

> Am 11.04.24 um 05:28 schrieb xinhui pan:
> > Ensure there is no address overlapping.
> >
> > Reported-by: Vlad Stolyarov <hexed at google.com>
> > Signed-off-by: xinhui pan <xinhui.pan at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 8af3f0fd3073..f1315a854192 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1852,6 +1852,12 @@ int amdgpu_vm_bo_clear_mappings(struct
> amdgpu_device *adev,
> >       LIST_HEAD(removed);
> >       uint64_t eaddr;
> >
> > +     /* validate the parameters */
> > +     if (saddr & ~PAGE_MASK || size & ~PAGE_MASK)
> > +             return -EINVAL;
>
> Well as general rule: *never* use PAGE_MASK and other PAGE_* macros
> here. This is GPUVM and not related to the CPUVM space.
>
> > +     if (saddr + size <= saddr)
> > +             return -EINVAL;
> > +
>
> Mhm, so basically size is not checked for a wraparound?
>
> >       eaddr = saddr + size - 1;
> >       saddr /= AMDGPU_GPU_PAGE_SIZE;
> >       eaddr /= AMDGPU_GPU_PAGE_SIZE;
>
> If that's the case then I would rather check for saddr < eaddr here.
>
> But that actually shouldn't matter since this code here:
>
>          /* Now gather all removed mappings */
>          tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
>          while (tmp) {
>
> Then shouldn't return anything, so the operation is basically a NO-OP then.
>
> Regards,
> Christian.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20240411/f04648d3/attachment-0001.htm>