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

[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.

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