Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems

["Christian König" <deathsimple@vodafone.de>]

Am 30.03.2017 um 03:41 schrieb Michel Dänzer:
> On 30/03/17 12:18 AM, Christian König wrote:
>> Am 29.03.2017 um 16:54 schrieb Michel Dänzer:
>>> On 29/03/17 10:22 PM, Christian König wrote:
>>>> Am 29.03.2017 um 11:18 schrieb Jan Burgmeier:
>>>>> Signed-off-by: Jan Burgmeier <jan.burgmeier@unicon-software.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 99424cb8020b..583d22974e14 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -908,6 +908,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                 struct amdgpu_bo *aobj = NULL;
>>>>>                 uint64_t offset;
>>>>>                 uint8_t *kptr;
>>>>> +            uint64_t it_last;
>>>>>                   m = amdgpu_cs_find_mapping(parser, chunk_ib->va_start,
>>>>>                                &aobj);
>>>>> @@ -916,8 +917,9 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device
>>>>> *adev,
>>>>>                     return -EINVAL;
>>>>>                 }
>>>>>     +            it_last = m->it.last;
>>>>>                 if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>> -                (m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>>> +                (it_last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>>> Nice catch, but just adding a u64 case should do here as well. E.g:
>>>>
>>>> if ((chunk_ib->va_start + chunk_ib->ib_bytes) >
>>>>       (u64)(m->it.last + 1) * AMDGPU_GPU_PAGE_SIZE) {
>>> That won't work correctly if m->it.last == 0xffffffff ? Or is that not
>>> possible?
>> Hui, why? is it.last signed?
> No. If m->it.last == 0xffffffff, (m->it.last + 1) == 0, the u64 cast
> won't change that. I thought that would be bad, but maybe not?

Ah, good catch. Yes the u64 cast needs to be inside the (), not outside.

Christian.