Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1611723 > unrolled thread
| Started by | Jan Burgmeier <jan.burgmeier@unicon-software.com> |
|---|---|
| First post | 2017-03-29 11:30 +0200 |
| Last post | 2017-03-30 10:40 +0200 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Jan Burgmeier <jan.burgmeier@unicon-software.com> - 2017-03-29 11:30 +0200
Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Christian König <deathsimple@vodafone.de> - 2017-03-29 15:30 +0200
Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Michel Dänzer <michel@daenzer.net> - 2017-03-29 17:00 +0200
Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Christian König <deathsimple@vodafone.de> - 2017-03-29 17:20 +0200
Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Michel Dänzer <michel@daenzer.net> - 2017-03-30 03:50 +0200
Re: [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems Christian König <deathsimple@vodafone.de> - 2017-03-30 10:40 +0200
| From | Jan Burgmeier <jan.burgmeier@unicon-software.com> |
|---|---|
| Date | 2017-03-29 11:30 +0200 |
| Subject | [PATCH] Fix IB va_start+ib_bytes range check on 32Bit systems |
| Message-ID | <tqi5s-4RU-21@gated-at.bofh.it> |
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) {
DRM_ERROR("IB va_start+ib_bytes is invalid\n");
return -EINVAL;
}
--
2.11.0
[toc] | [next] | [standalone]
| From | Christian König <deathsimple@vodafone.de> |
|---|---|
| Date | 2017-03-29 15:30 +0200 |
| Message-ID | <tqlPJ-7sg-31@gated-at.bofh.it> |
| In reply to | #1611723 |
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) {
With that fixed the patch is Reviewed-by: Christian König
<christian.koenig@amd.com>.
Regards,
Christian.
> DRM_ERROR("IB va_start+ib_bytes is invalid\n");
> return -EINVAL;
> }
[toc] | [prev] | [next] | [standalone]
| From | Michel Dänzer <michel@daenzer.net> |
|---|---|
| Date | 2017-03-29 17:00 +0200 |
| Message-ID | <tqneO-8iT-27@gated-at.bofh.it> |
| In reply to | #1611947 |
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?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
[toc] | [prev] | [next] | [standalone]
| From | Christian König <deathsimple@vodafone.de> |
|---|---|
| Date | 2017-03-29 17:20 +0200 |
| Message-ID | <tqnya-ea-33@gated-at.bofh.it> |
| In reply to | #1612007 |
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?
And even then m->it.last probably won't ever become 0xffffffff on a
32bit system.
BTW: We need to fix using the 64bit R/B tree instead of the long sized
tree for Vega10 here anyway.
Regards,
Christian.
[toc] | [prev] | [next] | [standalone]
| From | Michel Dänzer <michel@daenzer.net> |
|---|---|
| Date | 2017-03-30 03:50 +0200 |
| Message-ID | <tqxnP-7bX-1@gated-at.bofh.it> |
| In reply to | #1612041 |
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?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
[toc] | [prev] | [next] | [standalone]
| From | Christian König <deathsimple@vodafone.de> |
|---|---|
| Date | 2017-03-30 10:40 +0200 |
| Message-ID | <tqDMB-3Bj-1@gated-at.bofh.it> |
| In reply to | #1612488 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web