[PATCH] drm/amd/display: Implement bounds check for stream encoder creation in DCN301

[Li, Roman]

[Public]

Inline.

> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>
> Sent: Sunday, February 4, 2024 9:35 PM
> To: Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo
> <Aurabindo.Pillai at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM at amd.com>; Li, Roman <Roman.Li at amd.com>
> Subject: [PATCH] drm/amd/display: Implement bounds check for stream
> encoder creation in DCN301
>
> 'stream_enc_regs' array is an array of dcn10_stream_enc_registers structures.
> The array is initialized with four elements, corresponding to the four calls to
> stream_enc_regs() in the array initializer. This means that valid indices for this
> array are 0, 1, 2, and 3.
>
> The error message 'stream_enc_regs' 4 <= 5 below, is indicating that there is an
> attempt to access this array with an index of 5, which is out of bounds. This
> could lead to undefined behavior
>
> Here, eng_id is used as an index to access the stream_enc_regs array. If eng_id
> is 5, this would result in an out-of-bounds access.
>
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/../display/dc/resource/dcn301/dcn301_reso
> urce.c:1011 dcn301_stream_encoder_create() error: buffer overflow
> 'stream_enc_regs' 4 <= 5

Please mention that this is Smatch warning.
In current implementation this function is called with eng_id  limited by num_stream_encoder = 4 for dcn301.

>
> Fixes: 3a83e4e64bb1 ("drm/amd/display: Add dcn3.01 support to DC (v2)")
> Cc: Roman Li <Roman.Li at amd.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> Cc: Aurabindo Pillai <aurabindo.pillai at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  .../display/dc/resource/dcn301/dcn301_resource.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git
> a/drivers/gpu/drm/amd/display/dc/resource/dcn301/dcn301_resource.c
> b/drivers/gpu/drm/amd/display/dc/resource/dcn301/dcn301_resource.c
> index 511ff6b5b985..f915d7c3980e 100644
> --- a/drivers/gpu/drm/amd/display/dc/resource/dcn301/dcn301_resource.c
> +++
> b/drivers/gpu/drm/amd/display/dc/resource/dcn301/dcn301_resource.c
> @@ -1006,10 +1006,18 @@ static struct stream_encoder
> *dcn301_stream_encoder_create(enum engine_id eng_id
>               return NULL;
>       }

>
> -     dcn30_dio_stream_encoder_construct(enc1, ctx, ctx->dc_bios,
> -                                     eng_id, vpg, afmt,
> -                                     &stream_enc_regs[eng_id],
> -                                     &se_shift, &se_mask);
> +     if (eng_id < ARRAY_SIZE(stream_enc_regs)) {
> +             dcn30_dio_stream_encoder_construct(enc1, ctx, ctx-
> >dc_bios,
> +                                                eng_id, vpg, afmt,
> +                                                &stream_enc_regs[eng_id],
> +                                                &se_shift, &se_mask);
> +     } else {
> +             DRM_ERROR("Invalid engine id: %d\n", eng_id);
> +             kfree(enc1);
> +             kfree(vpg);
> +             kfree(afmt);
> +             return NULL;
> +     }

Can you just extend the existing null checks instead?
e.g.
        if (!enc1 || !vpg || !afmt || (eng_id  >= ARRAY_SIZE(stream_enc_regs))

>
>       return &enc1->base;
>  }
> --
> 2.34.1