+Jann Horn <ja...@google.com> for his thoughts
On Thu, Apr 11, 2024 at 12:25 PM Christian König <christian.koe...@amd.com>
wrote:
> Am 11.04.24 um 05:28 schrieb xinhui pan:
> > Ensure there is no address overlapping.
> >
> > Reported-by: Vlad Stolyarov <he...@google.com>
> > Signed-off-by: xinhui pan <xinhui....@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.
>
