[PATCH] drm/amdgpu: just disallow reading untouched registers

[PATCH] drm/amdgpu: just disallow reading untouched registers

[Christian König]

From: Christian König <christian.koenig@amd.com>

Not sure what the original intention was here, but returning a random piece of
kernel memory to userspace because we didn't set the value at all is clearly
not a good idea.

This patch disallows reading the register and returns
a proper error code instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 5c02ec4..f1c2bff 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct amdgpu_device *adev)
 }
 
 static const struct amdgpu_allowed_register_entry tonga_allowed_read_registers[] = {
-	{mmGB_MACROTILE_MODE7, true},
 };
 
 static const struct amdgpu_allowed_register_entry cz_allowed_read_registers[] = {
-	{mmGB_TILE_MODE7, true},
-	{mmGB_TILE_MODE12, true},
-	{mmGB_TILE_MODE17, true},
-	{mmGB_TILE_MODE23, true},
-	{mmGB_MACROTILE_MODE7, true},
 };
 
 static const struct amdgpu_allowed_register_entry vi_allowed_read_registers[] = {
-- 
2.5.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Christian König]

Ping!

Does anybody of you guys know what the background of those "untouched" 
registers is?

That we leak uninitialized memory to userspace is a bit bad.

Thanks,
Christian.

Am 28.03.2017 um 13:24 schrieb Christian König:
> From: Christian König <christian.koenig@amd.com>
>
> Not sure what the original intention was here, but returning a random piece of
> kernel memory to userspace because we didn't set the value at all is clearly
> not a good idea.
>
> This patch disallows reading the register and returns
> a proper error code instead.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>   1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 5c02ec4..f1c2bff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct amdgpu_device *adev)
>   }
>   
>   static const struct amdgpu_allowed_register_entry tonga_allowed_read_registers[] = {
> -	{mmGB_MACROTILE_MODE7, true},
>   };
>   
>   static const struct amdgpu_allowed_register_entry cz_allowed_read_registers[] = {
> -	{mmGB_TILE_MODE7, true},
> -	{mmGB_TILE_MODE12, true},
> -	{mmGB_TILE_MODE17, true},
> -	{mmGB_TILE_MODE23, true},
> -	{mmGB_MACROTILE_MODE7, true},
>   };
>   
>   static const struct amdgpu_allowed_register_entry vi_allowed_read_registers[] = {


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Tom St Denis]

On 29/03/17 08:18 AM, Christian König wrote:
> Ping!
>
> Does anybody of you guys know what the background of those "untouched"
> registers is?
>
> That we leak uninitialized memory to userspace is a bit bad.

I don't know the history but wouldn't reading these just read the MMIO 
registers?  Or these are cached and we're reading kernel mem?

It seems sensible to remove them though and the patch looks clean so 
I'll give an ack-by.

Tom

>
> Thanks,
> Christian.
>
> Am 28.03.2017 um 13:24 schrieb Christian König:
>> From: Christian König <christian.koenig@amd.com>
>>
>> Not sure what the original intention was here, but returning a random
>> piece of
>> kernel memory to userspace because we didn't set the value at all is
>> clearly
>> not a good idea.
>>
>> This patch disallows reading the register and returns
>> a proper error code instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>> b/drivers/gpu/drm/amd/amdgpu/vi.c
>> index 5c02ec4..f1c2bff 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct
>> amdgpu_device *adev)
>>   }
>>     static const struct amdgpu_allowed_register_entry
>> tonga_allowed_read_registers[] = {
>> -    {mmGB_MACROTILE_MODE7, true},
>>   };
>>     static const struct amdgpu_allowed_register_entry
>> cz_allowed_read_registers[] = {
>> -    {mmGB_TILE_MODE7, true},
>> -    {mmGB_TILE_MODE12, true},
>> -    {mmGB_TILE_MODE17, true},
>> -    {mmGB_TILE_MODE23, true},
>> -    {mmGB_MACROTILE_MODE7, true},
>>   };
>>     static const struct amdgpu_allowed_register_entry
>> vi_allowed_read_registers[] = {
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Christian König]

Am 29.03.2017 um 14:28 schrieb Tom St Denis:
> On 29/03/17 08:18 AM, Christian König wrote:
>> Ping!
>>
>> Does anybody of you guys know what the background of those "untouched"
>> registers is?
>>
>> That we leak uninitialized memory to userspace is a bit bad.
>
> I don't know the history but wouldn't reading these just read the MMIO 
> registers?  Or these are cached and we're reading kernel mem?

The crux is we actually don't read them, but just ignore them.

In other words we copy back random kernel memory to userspace because we 
didn't initialize the returned value.

That is strange at best, but I can't figure out the background here.

Christian.

>
> It seems sensible to remove them though and the patch looks clean so 
> I'll give an ack-by.
>
> Tom
>
>>
>> Thanks,
>> Christian.
>>
>> Am 28.03.2017 um 13:24 schrieb Christian König:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Not sure what the original intention was here, but returning a random
>>> piece of
>>> kernel memory to userspace because we didn't set the value at all is
>>> clearly
>>> not a good idea.
>>>
>>> This patch disallows reading the register and returns
>>> a proper error code instead.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>>>   1 file changed, 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> index 5c02ec4..f1c2bff 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct
>>> amdgpu_device *adev)
>>>   }
>>>     static const struct amdgpu_allowed_register_entry
>>> tonga_allowed_read_registers[] = {
>>> -    {mmGB_MACROTILE_MODE7, true},
>>>   };
>>>     static const struct amdgpu_allowed_register_entry
>>> cz_allowed_read_registers[] = {
>>> -    {mmGB_TILE_MODE7, true},
>>> -    {mmGB_TILE_MODE12, true},
>>> -    {mmGB_TILE_MODE17, true},
>>> -    {mmGB_TILE_MODE23, true},
>>> -    {mmGB_MACROTILE_MODE7, true},
>>>   };
>>>     static const struct amdgpu_allowed_register_entry
>>> vi_allowed_read_registers[] = {
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Alex Deucher]

On Wed, Mar 29, 2017 at 8:36 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 29.03.2017 um 14:28 schrieb Tom St Denis:
>>
>> On 29/03/17 08:18 AM, Christian König wrote:
>>>
>>> Ping!
>>>
>>> Does anybody of you guys know what the background of those "untouched"
>>> registers is?
>>>
>>> That we leak uninitialized memory to userspace is a bit bad.
>>
>>
>> I don't know the history but wouldn't reading these just read the MMIO
>> registers?  Or these are cached and we're reading kernel mem?
>
>
> The crux is we actually don't read them, but just ignore them.
>
> In other words we copy back random kernel memory to userspace because we
> didn't initialize the returned value.

Well, it's zeroed memory, so it's not random.  They just return 0.

>
> That is strange at best, but I can't figure out the background here.

No idea why they were added separately.  AFAIK, those indices are not used.

Alex

>
> Christian.
>
>
>>
>> It seems sensible to remove them though and the patch looks clean so I'll
>> give an ack-by.
>>
>> Tom
>>
>>>
>>> Thanks,
>>> Christian.
>>>
>>> Am 28.03.2017 um 13:24 schrieb Christian König:
>>>>
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> Not sure what the original intention was here, but returning a random
>>>> piece of
>>>> kernel memory to userspace because we didn't set the value at all is
>>>> clearly
>>>> not a good idea.
>>>>
>>>> This patch disallows reading the register and returns
>>>> a proper error code instead.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>>>>   1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> index 5c02ec4..f1c2bff 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct
>>>> amdgpu_device *adev)
>>>>   }
>>>>     static const struct amdgpu_allowed_register_entry
>>>> tonga_allowed_read_registers[] = {
>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>   };
>>>>     static const struct amdgpu_allowed_register_entry
>>>> cz_allowed_read_registers[] = {
>>>> -    {mmGB_TILE_MODE7, true},
>>>> -    {mmGB_TILE_MODE12, true},
>>>> -    {mmGB_TILE_MODE17, true},
>>>> -    {mmGB_TILE_MODE23, true},
>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>   };
>>>>     static const struct amdgpu_allowed_register_entry
>>>> vi_allowed_read_registers[] = {
>>>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Christian König]

Am 29.03.2017 um 15:29 schrieb Alex Deucher:
> On Wed, Mar 29, 2017 at 8:36 AM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Am 29.03.2017 um 14:28 schrieb Tom St Denis:
>>> On 29/03/17 08:18 AM, Christian König wrote:
>>>> Ping!
>>>>
>>>> Does anybody of you guys know what the background of those "untouched"
>>>> registers is?
>>>>
>>>> That we leak uninitialized memory to userspace is a bit bad.
>>>
>>> I don't know the history but wouldn't reading these just read the MMIO
>>> registers?  Or these are cached and we're reading kernel mem?
>>
>> The crux is we actually don't read them, but just ignore them.
>>
>> In other words we copy back random kernel memory to userspace because we
>> didn't initialize the returned value.
> Well, it's zeroed memory, so it's not random.  They just return 0.

No, the IOCTL is using kmalloc_array, not kcalloc or kzalloc.

That's why I stumbled over it in the first place.

>
>> That is strange at best, but I can't figure out the background here.
> No idea why they were added separately.  AFAIK, those indices are not used.

Dare to put an rb or ack-by on it?

Christian.

>
> Alex
>
>> Christian.
>>
>>
>>> It seems sensible to remove them though and the patch looks clean so I'll
>>> give an ack-by.
>>>
>>> Tom
>>>
>>>> Thanks,
>>>> Christian.
>>>>
>>>> Am 28.03.2017 um 13:24 schrieb Christian König:
>>>>> From: Christian König <christian.koenig@amd.com>
>>>>>
>>>>> Not sure what the original intention was here, but returning a random
>>>>> piece of
>>>>> kernel memory to userspace because we didn't set the value at all is
>>>>> clearly
>>>>> not a good idea.
>>>>>
>>>>> This patch disallows reading the register and returns
>>>>> a proper error code instead.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>> index 5c02ec4..f1c2bff 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct
>>>>> amdgpu_device *adev)
>>>>>    }
>>>>>      static const struct amdgpu_allowed_register_entry
>>>>> tonga_allowed_read_registers[] = {
>>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>>    };
>>>>>      static const struct amdgpu_allowed_register_entry
>>>>> cz_allowed_read_registers[] = {
>>>>> -    {mmGB_TILE_MODE7, true},
>>>>> -    {mmGB_TILE_MODE12, true},
>>>>> -    {mmGB_TILE_MODE17, true},
>>>>> -    {mmGB_TILE_MODE23, true},
>>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>>    };
>>>>>      static const struct amdgpu_allowed_register_entry
>>>>> vi_allowed_read_registers[] = {
>>>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: just disallow reading untouched registers

[Alex Deucher]

On Wed, Mar 29, 2017 at 9:46 AM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 29.03.2017 um 15:29 schrieb Alex Deucher:
>>
>> On Wed, Mar 29, 2017 at 8:36 AM, Christian König
>> <deathsimple@vodafone.de> wrote:
>>>
>>> Am 29.03.2017 um 14:28 schrieb Tom St Denis:
>>>>
>>>> On 29/03/17 08:18 AM, Christian König wrote:
>>>>>
>>>>> Ping!
>>>>>
>>>>> Does anybody of you guys know what the background of those "untouched"
>>>>> registers is?
>>>>>
>>>>> That we leak uninitialized memory to userspace is a bit bad.
>>>>
>>>>
>>>> I don't know the history but wouldn't reading these just read the MMIO
>>>> registers?  Or these are cached and we're reading kernel mem?
>>>
>>>
>>> The crux is we actually don't read them, but just ignore them.
>>>
>>> In other words we copy back random kernel memory to userspace because we
>>> didn't initialize the returned value.
>>
>> Well, it's zeroed memory, so it's not random.  They just return 0.
>
>
> No, the IOCTL is using kmalloc_array, not kcalloc or kzalloc.
>
> That's why I stumbled over it in the first place.

But the register is read from the cached copy which is zeroed.

>
>>
>>> That is strange at best, but I can't figure out the background here.
>>
>> No idea why they were added separately.  AFAIK, those indices are not
>> used.
>
>
> Dare to put an rb or ack-by on it?

Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
> Christian.
>
>
>>
>> Alex
>>
>>> Christian.
>>>
>>>
>>>> It seems sensible to remove them though and the patch looks clean so
>>>> I'll
>>>> give an ack-by.
>>>>
>>>> Tom
>>>>
>>>>> Thanks,
>>>>> Christian.
>>>>>
>>>>> Am 28.03.2017 um 13:24 schrieb Christian König:
>>>>>>
>>>>>> From: Christian König <christian.koenig@amd.com>
>>>>>>
>>>>>> Not sure what the original intention was here, but returning a random
>>>>>> piece of
>>>>>> kernel memory to userspace because we didn't set the value at all is
>>>>>> clearly
>>>>>> not a good idea.
>>>>>>
>>>>>> This patch disallows reading the register and returns
>>>>>> a proper error code instead.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/vi.c | 6 ------
>>>>>>    1 file changed, 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>>> index 5c02ec4..f1c2bff 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
>>>>>> @@ -465,15 +465,9 @@ static void vi_detect_hw_virtualization(struct
>>>>>> amdgpu_device *adev)
>>>>>>    }
>>>>>>      static const struct amdgpu_allowed_register_entry
>>>>>> tonga_allowed_read_registers[] = {
>>>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>>>    };
>>>>>>      static const struct amdgpu_allowed_register_entry
>>>>>> cz_allowed_read_registers[] = {
>>>>>> -    {mmGB_TILE_MODE7, true},
>>>>>> -    {mmGB_TILE_MODE12, true},
>>>>>> -    {mmGB_TILE_MODE17, true},
>>>>>> -    {mmGB_TILE_MODE23, true},
>>>>>> -    {mmGB_MACROTILE_MODE7, true},
>>>>>>    };
>>>>>>      static const struct amdgpu_allowed_register_entry
>>>>>> vi_allowed_read_registers[] = {
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx