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

Wed, 30 May 2018 01:27:01 -0700 

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:[email protected]]
Sent: Tuesday, May 29, 2018 11:50 PM
To: Quan, Evan <[email protected]>
Cc: amd-gfx list <[email protected]>; Deucher, Alexander
<[email protected]>; Huang, Ray <[email protected]>
Subject: Re: [PATCH] drm/amdgpu: detect buffer overflow and avoid
unnecessary dereference

On Tue, May 29, 2018 at 6:17 AM, Evan Quan <[email protected]> wrote:

Change-Id: I6666d7dcf60acf524f290460d2ffe3f1f5f46354
Signed-off-by: Evan Quan <[email protected]>

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to