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

[Li, Roman]

[Public]

Inline.

> -Original Message-
> From: SHANMUGAM, SRINIVASAN 
> Sent: Monday, February 5, 2024 10:47 PM
> To: Li, Roman ; Siqueira, Rodrigo
> ; Pillai, Aurabindo 
> Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> 
> Subject: [PATCH v2] 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 on the stream_enc_regs
> array.
>
> Thus fixing Buffer overflow error in dcn301_stream_encoder_create reported
> by Smatch:
> 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
>
> Fixes: 3a83e4e64bb1 ("drm/amd/display: Add dcn3.01 support to DC (v2)")
> Cc: Roman Li 
> Cc: Rodrigo Siqueira 
> Cc: Aurabindo Pillai 
> Signed-off-by: Srinivasan Shanmugam 
> ---
>  .../drm/amd/display/dc/resource/dcn301/dcn301_resource.c | 9 -
>  1 file changed, 4 insertions(+), 5 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..4a475a723191 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
> @@ -999,7 +999,7 @@ static struct stream_encoder
> *dcn301_stream_encoder_create(enum engine_id eng_id
>   vpg = dcn301_vpg_create(ctx, vpg_inst);
>   afmt = dcn301_afmt_create(ctx, afmt_inst);
>
> - if (!enc1 || !vpg || !afmt) {
> + if (!enc1 || !vpg || !afmt || eng_id >= ARRAY_SIZE(stream_enc_regs))
> {
>   kfree(enc1);
>   kfree(vpg);
>   kfree(afmt);

Reviewed-by: Roman Li 
I don't think the part below is necessary.

> @@ -1007,10 +1007,9 @@ static struct stream_encoder
> *dcn301_stream_encoder_create(enum engine_id eng_id
>   }
>
>   dcn30_dio_stream_encoder_construct(enc1, ctx, ctx->dc_bios,
> - eng_id, vpg, afmt,
> - _enc_regs[eng_id],
> - _shift, _mask);
> -
> +eng_id, vpg, afmt,
> +_enc_regs[eng_id],
> +_shift, _mask);
>   return >base;
>  }
>
> --
> 2.34.1

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

[Srinivasan Shanmugam]

'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 on the
stream_enc_regs array.

Thus fixing Buffer overflow error in dcn301_stream_encoder_create
reported by Smatch:
drivers/gpu/drm/amd/amdgpu/../display/dc/resource/dcn301/dcn301_resource.c:1011 
dcn301_stream_encoder_create() error: buffer overflow 'stream_enc_regs' 4 <= 5

Fixes: 3a83e4e64bb1 ("drm/amd/display: Add dcn3.01 support to DC (v2)")
Cc: Roman Li 
Cc: Rodrigo Siqueira 
Cc: Aurabindo Pillai 
Signed-off-by: Srinivasan Shanmugam 
---
 .../drm/amd/display/dc/resource/dcn301/dcn301_resource.c | 9 -
 1 file changed, 4 insertions(+), 5 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..4a475a723191 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
@@ -999,7 +999,7 @@ static struct stream_encoder 
*dcn301_stream_encoder_create(enum engine_id eng_id
vpg = dcn301_vpg_create(ctx, vpg_inst);
afmt = dcn301_afmt_create(ctx, afmt_inst);
 
-   if (!enc1 || !vpg || !afmt) {
+   if (!enc1 || !vpg || !afmt || eng_id >= ARRAY_SIZE(stream_enc_regs)) {
kfree(enc1);
kfree(vpg);
kfree(afmt);
@@ -1007,10 +1007,9 @@ static struct stream_encoder 
*dcn301_stream_encoder_create(enum engine_id eng_id
}
 
dcn30_dio_stream_encoder_construct(enc1, ctx, ctx->dc_bios,
-   eng_id, vpg, afmt,
-   _enc_regs[eng_id],
-   _shift, _mask);
-
+  eng_id, vpg, afmt,
+  _enc_regs[eng_id],
+  _shift, _mask);
return >base;
 }
 
-- 
2.34.1