[PATCH] drm/amdgpu: Fix potential out-of-bounds access in 'amdgpu_discovery_reg_base_init()'

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>
> Sent: Thursday, February 1, 2024 12:36 PM
> To: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM at amd.com>
> 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 at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  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