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

[Alex Deucher]

On Wed, Mar 29, 2017 at 8:36 AM, Christian König
<deathsimple at 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 at 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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx