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

[Jann Horn]

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?

Yeah, exactly.

> >       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.

FWIW, it would probably a good idea to keep the added check analogous
to other functions called from amdgpu_gem_va_ioctl() like
amdgpu_vm_bo_replace_map(), which also checks "if (saddr + size <=
saddr || offset + size <= offset)" before the division.

> 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.

That's not how it works; the interval tree is not designed to be fed
bogus ranges that end before they start. (Or at least I don't think it
is - if it is, it is buggy.) I think basically if the specified start
and end addresses are both within an rbtree node, this rbtree node is
returned from the lookup, even if the specified end address is before
the specified start address.

A more verbose example:
Let's assume the interval tree contains a single entry from address A
to address D.
Looking at the _iter_first implementation in interval_tree_generic.h,
when it is called with a start address C which is between A and D, and
an end address B (result of an addition that wraps around to an
address below C but above A), it does the following:

1. bails out if "node->ITSUBTREE < start" (meaning if the specified
start address C is after the range covered by the root node - which is
not the case)
2. bails out if "ITSTART(leftmost) > last" (meaning if the specified
end address is smaller than the entry start address A - which is not
the case)
3. enters _subtree_search. in there:
4. the root node has no children, so "node->ITRB.rb_left" is NULL
5. the specified end address B is after the node's start address A, so
"Cond1" is fulfilled
6. the specified start address C is before the node's end address D,
so "Cond2" is fulfilled
7. the root node is returned from the lookup