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
