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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <df03b893-63eb-de37-28fd-a8f423ed95ff@vodafone.de>
Date:   Thu, 30 Mar 2017 10:36:30 +0200
From:   Christian König <deathsimple@...afone.de>
To:     Michel Dänzer <michel@...nzer.net>,
        Jan Burgmeier <jan.burgmeier@...con-software.com>
Cc:     airlied@...ux.ie, dri-devel@...ts.freedesktop.org,
        amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems

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