> 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
