[PATCH] drm/amdgpu: detect buffer overflow and avoid unnecessary dereference

[Christian König]

Am 30.05.2018 um 06:20 schrieb Quan, Evan:
>> Maybe this should be a WARN_ON and then we clamp the range?
>>
> According to the spec, it should store all indirect_start_offsets into the array.
> And the current array should be enough.
> So, if overflow occurred, it should be a bug case and BUG_ON seems more proper.

I have no idea what the code does, but BUG_ON is usually only justified 
if you need to prevent data loss or system corruption (or run into a 
NULL pointer exception later on anyway or other stuff like that).

If you can safely abort whatever the code does and return an error then 
WARN_ON is more appropriate.

Regards,
Christian.

>
> Regards,
> Evan
>> -----Original Message-----
>> From: Alex Deucher [mailto:alexdeucher at gmail.com]
>> Sent: Tuesday, May 29, 2018 11:50 PM
>> To: Quan, Evan <Evan.Quan at amd.com>
>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Deucher, Alexander
>> <Alexander.Deucher at amd.com>; Huang, Ray <Ray.Huang at amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: detect buffer overflow and avoid
>> unnecessary dereference
>>
>> On Tue, May 29, 2018 at 6:17 AM, Evan Quan <evan.quan at amd.com> wrote:
>>> Change-Id: I6666d7dcf60acf524f290460d2ffe3f1f5f46354
>>> Signed-off-by: Evan Quan <evan.quan at amd.com>
>> Please include a patch description as well.  One comment below.
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 15 +++++++++------
>>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> index 7c5a850..5a86726 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> @@ -1838,13 +1838,15 @@ static void gfx_v9_1_parse_ind_reg_list(int
>> *register_list_format,
>>>                                  int indirect_offset,
>>>                                  int list_size,
>>>                                  int *unique_indirect_regs,
>>> -                               int *unique_indirect_reg_count,
>>> +                               int unique_indirect_reg_count,
>>>                                  int *indirect_start_offsets,
>>> -                               int *indirect_start_offsets_count)
>>> +                               int *indirect_start_offsets_count,
>>> +                               int max_start_offsets_count)
>>>   {
>>>          int idx;
>>>
>>>          for (; indirect_offset < list_size; indirect_offset++) {
>>> +               BUG_ON(*indirect_start_offsets_count >=
>> max_start_offsets_count);
>>
>> Maybe this should be a WARN_ON and then we clamp the range?
>>
>> Alex
>>
>>>                  indirect_start_offsets[*indirect_start_offsets_count] =
>> indirect_offset;
>>>                  *indirect_start_offsets_count = *indirect_start_offsets_count + 1;
>>>
>>> @@ -1852,14 +1854,14 @@ static void gfx_v9_1_parse_ind_reg_list(int
>> *register_list_format,
>>>                          indirect_offset += 2;
>>>
>>>                          /* look for the matching indice */
>>> -                       for (idx = 0; idx < *unique_indirect_reg_count; idx++) {
>>> +                       for (idx = 0; idx < unique_indirect_reg_count; idx++) {
>>>                                  if (unique_indirect_regs[idx] ==
>>>                                          register_list_format[indirect_offset] ||
>>>                                          !unique_indirect_regs[idx])
>>>                                          break;
>>>                          }
>>>
>>> -                       BUG_ON(idx >= *unique_indirect_reg_count);
>>> +                       BUG_ON(idx >= unique_indirect_reg_count);
>>>
>>>                          if (!unique_indirect_regs[idx])
>>>                                  unique_indirect_regs[idx] =
>> register_list_format[indirect_offset];
>>> @@ -1894,9 +1896,10 @@ static int
>> gfx_v9_1_init_rlc_save_restore_list(struct amdgpu_device *adev)
>>>                                      adev->gfx.rlc.reg_list_format_direct_reg_list_length,
>>>                                      adev->gfx.rlc.reg_list_format_size_bytes >> 2,
>>>                                      unique_indirect_regs,
>>> -                                   &unique_indirect_reg_count,
>>> +                                   unique_indirect_reg_count,
>>>                                      indirect_start_offsets,
>>> -                                   &indirect_start_offsets_count);
>>> +                                   &indirect_start_offsets_count,
>>> +                                   ARRAY_SIZE(indirect_start_offsets));
>>>
>>>          /* enable auto inc in case it is disabled */
>>>          tmp = RREG32(SOC15_REG_OFFSET(GC, 0, mmRLC_SRM_CNTL));
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> 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