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

[Christian König]

Am 11.04.24 um 17:44 schrieb 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.

I would also change that function as well.

The eaddr needs to be checked against the max_pfn as well and we 
currently shift that around for this check which looks quite ugly.

Only the overflow check can probably be before it.

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

Ah, yeah that makes sense. The search functions checks if a node only 
partially intersects with start and end and not if it is covered by it.

Thanks,
Christian.

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