[Public]
> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>
> Sent: Thursday, February 1, 2024 12:36 PM
> To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@xxxxxxx>
> Subject: [PATCH] drm/amdgpu: Fix potential out-of-bounds access in
> 'amdgpu_discovery_reg_base_init()'
>
> The issue arises when the array 'adev->vcn.vcn_config' is accessed before
> checking if the index 'adev->vcn.num_vcn_inst' is within the bounds of the
> array.
>
> The fix involves moving the bounds check before the array access. This ensures
> that 'adev->vcn.num_vcn_inst' is within the bounds of the array before it is
> used as an index.
>
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1289
> amdgpu_discovery_reg_base_init() error: testing array offset 'adev-
> >vcn.num_vcn_inst' after use.
>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index ef800590c1ab..83da46d73f70 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -1282,11 +1282,11 @@ static int
> amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> * 0b10 : encode is disabled
> * 0b01 : decode is disabled
> */
> - adev->vcn.vcn_config[adev-
> >vcn.num_vcn_inst] =
> - ip->revision & 0xc0;
> - ip->revision &= ~0xc0;
> if (adev->vcn.num_vcn_inst <
> AMDGPU_MAX_VCN_INSTANCES) {
> + adev->vcn.vcn_config[adev-
> >vcn.num_vcn_inst] =
> + ip->revision & 0xc0;
> + ip->revision &= ~0xc0;
I have vague recollections of this being this way for a reason, but I can't recall why at this time. That said, the ` ip->revision &= ~0xc0;` should always be executed, not just if the number of instances < MAX_VCN_INSTANCES. So I would move that line after the if/else block.
Alex
> adev->vcn.num_vcn_inst++;
> adev->vcn.inst_mask |=
> (1U << ip->instance_number);
> --
> 2.34.1
