[PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Tom St Denis]

We don't need to check the alignment of the offset and there was
potential a buffer overflow as well.

Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index c98e59721444..b1ea300008e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct file *f, char __user *buf,
 	return result;
 }
 
+// read at most 1024 words
+#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
+
 static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
 					size_t size, loff_t *pos)
 {
@@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
 	ssize_t result = 0;
 	uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
 
-	if (size & 3 || *pos & 3)
+	if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
 		return -EINVAL;
 
 	/* decode offset */
@@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
 	thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
 	bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
 
-	data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
+	data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
+						 GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-- 
2.14.3

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

Re: [PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Tom St Denis]

Ping?

On 04/09/2018 08:16 AM, Tom St Denis wrote:
> We don't need to check the alignment of the offset and there was
> potential a buffer overflow as well.
> 
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index c98e59721444..b1ea300008e5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct file *f, char __user *buf,
>   	return result;
>   }
>   
> +// read at most 1024 words
> +#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
> +
>   static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
>   					size_t size, loff_t *pos)
>   {
> @@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
>   	ssize_t result = 0;
>   	uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
>   
> -	if (size & 3 || *pos & 3)
> +	if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>   		return -EINVAL;
>   
>   	/* decode offset */
> @@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
>   	thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
>   	bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
>   
> -	data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
> +	data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
> +						 GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
>   
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Christian König]

>
>   -    if (size & 3 || *pos & 3)
> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ)) 

I think checking the position alignment here is still necessary, cause 
we can't read from not dw boundaries don't we?

Christian.

Am 11.04.2018 um 13:55 schrieb Tom St Denis:
> Ping?
>
> On 04/09/2018 08:16 AM, Tom St Denis wrote:
>> We don't need to check the alignment of the offset and there was
>> potential a buffer overflow as well.
>>
>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index c98e59721444..b1ea300008e5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
>> file *f, char __user *buf,
>>       return result;
>>   }
>>   +// read at most 1024 words
>> +#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
>> +
>>   static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user 
>> *buf,
>>                       size_t size, loff_t *pos)
>>   {
>> @@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>> file *f, char __user *buf,
>>       ssize_t result = 0;
>>       uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
>>   -    if (size & 3 || *pos & 3)
>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>>           return -EINVAL;
>>         /* decode offset */
>> @@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>> file *f, char __user *buf,
>>       thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
>>       bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
>>   -    data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
>> +    data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
>> +                         GFP_KERNEL);
>>       if (!data)
>>           return -ENOMEM;
>>
> _______________________________________________
> 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/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Tom St Denis]

On 04/11/2018 07:58 AM, Christian König wrote:
>>
>>   -    if (size & 3 || *pos & 3)
>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ)) 
> 
> I think checking the position alignment here is still necessary, cause 
> we can't read from not dw boundaries don't we?

The index is a dword index as fed into SQ_IND_INDEX (offset => start => 
regno as you trace from the debugfs entry to gfx_v8_0_read_wave_sgprs to 
wave_read_regs (or analogues...)).

SQ_IND_INDEX doesn't take a byte offset but a dword offset, for instance:

include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP0 
                                                                   0x026c
include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP1 
                                                                   0x026d
include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP2 
                                                                   0x026e


The current way for instance would prohibit reading (directly) 
SQ_WAVE_TTMP1.

I agree it's not really how a typical file device works but it's not a 
typical file device :-).  It's assumed every read would be preceded by a 
seek to set the higher order bits anyways.

Cheers,
Tom


> 
> Christian.
> 
> Am 11.04.2018 um 13:55 schrieb Tom St Denis:
>> Ping?
>>
>> On 04/09/2018 08:16 AM, Tom St Denis wrote:
>>> We don't need to check the alignment of the offset and there was
>>> potential a buffer overflow as well.
>>>
>>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index c98e59721444..b1ea300008e5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
>>> file *f, char __user *buf,
>>>       return result;
>>>   }
>>>   +// read at most 1024 words
>>> +#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
>>> +
>>>   static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user 
>>> *buf,
>>>                       size_t size, loff_t *pos)
>>>   {
>>> @@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>>> file *f, char __user *buf,
>>>       ssize_t result = 0;
>>>       uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
>>>   -    if (size & 3 || *pos & 3)
>>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>>>           return -EINVAL;
>>>         /* decode offset */
>>> @@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>>> file *f, char __user *buf,
>>>       thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
>>>       bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
>>>   -    data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
>>> +    data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
>>> +                         GFP_KERNEL);
>>>       if (!data)
>>>           return -ENOMEM;
>>>
>> _______________________________________________
>> 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/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Christian König]

Am 11.04.2018 um 14:03 schrieb Tom St Denis:
>
>
> On 04/11/2018 07:58 AM, Christian König wrote:
>>>
>>>   -    if (size & 3 || *pos & 3)
>>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ)) 
>>
>> I think checking the position alignment here is still necessary, 
>> cause we can't read from not dw boundaries don't we?
>
> The index is a dword index as fed into SQ_IND_INDEX (offset => start 
> => regno as you trace from the debugfs entry to 
> gfx_v8_0_read_wave_sgprs to wave_read_regs (or analogues...)).
>
> SQ_IND_INDEX doesn't take a byte offset but a dword offset, for instance:
>
> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP0 0x026c
> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP1 0x026d
> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP2 0x026e
>
>
> The current way for instance would prohibit reading (directly) 
> SQ_WAVE_TTMP1.
>
> I agree it's not really how a typical file device works but it's not a 
> typical file device :-).  It's assumed every read would be preceded by 
> a seek to set the higher order bits anyways.

So pos=0 is one dw and pos=1 is another one? If that's the case then 
that would be a bug since pos is by definition a byte offset.

I suggest to keep the limitation and instead fix how pos is interpreted 
instead.

Regards,
Christian.

>
> Cheers,
> Tom
>
>
>>
>> Christian.
>>
>> Am 11.04.2018 um 13:55 schrieb Tom St Denis:
>>> Ping?
>>>
>>> On 04/09/2018 08:16 AM, Tom St Denis wrote:
>>>> We don't need to check the alignment of the offset and there was
>>>> potential a buffer overflow as well.
>>>>
>>>> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index c98e59721444..b1ea300008e5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
>>>> file *f, char __user *buf,
>>>>       return result;
>>>>   }
>>>>   +// read at most 1024 words
>>>> +#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
>>>> +
>>>>   static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char 
>>>> __user *buf,
>>>>                       size_t size, loff_t *pos)
>>>>   {
>>>> @@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>>>> file *f, char __user *buf,
>>>>       ssize_t result = 0;
>>>>       uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
>>>>   -    if (size & 3 || *pos & 3)
>>>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>>>>           return -EINVAL;
>>>>         /* decode offset */
>>>> @@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct 
>>>> file *f, char __user *buf,
>>>>       thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
>>>>       bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
>>>>   -    data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
>>>> +    data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
>>>> +                         GFP_KERNEL);
>>>>       if (!data)
>>>>           return -ENOMEM;
>>>>
>>> _______________________________________________
>>> 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/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Tom St Denis]

On 04/11/2018 08:17 AM, Christian König wrote:
> Am 11.04.2018 um 14:03 schrieb Tom St Denis:
>>
>>
>> On 04/11/2018 07:58 AM, Christian König wrote:
>>>>
>>>>   -    if (size & 3 || *pos & 3)
>>>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ)) 
>>>
>>> I think checking the position alignment here is still necessary, 
>>> cause we can't read from not dw boundaries don't we?
>>
>> The index is a dword index as fed into SQ_IND_INDEX (offset => start 
>> => regno as you trace from the debugfs entry to 
>> gfx_v8_0_read_wave_sgprs to wave_read_regs (or analogues...)).
>>
>> SQ_IND_INDEX doesn't take a byte offset but a dword offset, for instance:
>>
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP0 0x026c
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP1 0x026d
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP2 0x026e
>>
>>
>> The current way for instance would prohibit reading (directly) 
>> SQ_WAVE_TTMP1.
>>
>> I agree it's not really how a typical file device works but it's not a 
>> typical file device :-).  It's assumed every read would be preceded by 
>> a seek to set the higher order bits anyways.
> 
> So pos=0 is one dw and pos=1 is another one? If that's the case then 
> that would be a bug since pos is by definition a byte offset.
> 
> I suggest to keep the limitation and instead fix how pos is interpreted 
> instead.

It would break copies of umr out there already (in ways that won't be 
obvious and therefore result in lost time/etc).   Since umr is probably 
the only user of this it's not really a big deal.  Ideally, everyone 
uses the latest umr each day :-) but that's not realistically the case.

I agree that had I written this today I would keep the file offset 
consistent with the byte offset into the register file (shift down by 4 
before calling the callback).

Since you need to seek to a very specific address to use this device 
it's not the sort of thing that someone would cat and then expect 
sensible output anyways.

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