On 2026. július 15., szerda 13:10:51 közép-európai nyári idő sashiko-
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider: - [High] Uninitialized variable `block_size_log2` in
> GFX6 modifier path causes undefined behavior and potential kernel panic. -
> [High] User-controlled `PIPE_CONFIG` in GFX6 modifier can trigger
> `unreachable()`, leading to kernel panic or undefined behavior. - [Medium]
> Unbounded `drm_err()` on invalid GFX6 array mode allows userspace to flood
> the kernel log. --
>
> --- Patch [2]: [PATCH 2/4] drm/amdgpu: Convert tiling flags to modifiers on
> GFX6-8 --- commit 642125971654c581668fe700356dfbee6905eccc
> Author: Timur Kristóf <[email protected]>
> Subject: drm/amdgpu: Convert tiling flags to modifiers on GFX6-8
>
> This commit converts the tiling flags to modifiers on GFX8 and older,
> so that the DC display driver can rely on them like on newer generations.
>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index
> > 950937b363fbe..dcca930ed3c04 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>
> [ ... ]
>
> > @@ -940,6 +940,55 @@ static int convert_tiling_flags_to_modifier(struct
> > amdgpu_framebuffer *afb)>
> > return 0;
> >
> > }
> >
> > +static int convert_tiling_flags_to_modifier_gfx6(struct
> > amdgpu_framebuffer *afb) +{
> > + const uint32_t array_mode = AMDGPU_TILING_GET(afb->tiling_flags,
> > ARRAY_MODE); + const uint32_t pipe_config =
> > AMDGPU_TILING_GET(afb->tiling_flags, PIPE_CONFIG);
> [ ... ]
>
> > + switch (array_mode) {
> > + case DC_ARRAY_LINEAR_GENERAL:
> > + case DC_ARRAY_LINEAR_ALLIGNED:
> > + modifier = DRM_FORMAT_MOD_LINEAR;
> > + break;
> > +
> > + case DC_ARRAY_2D_TILED_THIN1:
> > + /* Macro tiled modes only */
> > + modifier |=
> > + AMD_FMT_MOD_SET(PIPE_CONFIG, pipe_config) |
> > + AMD_FMT_MOD_SET(TILE_SPLIT, tile_split) |
> > + AMD_FMT_MOD_SET(BANK_WIDTH, bank_width) |
> > + AMD_FMT_MOD_SET(BANK_HEIGHT, bank_height) |
> > + AMD_FMT_MOD_SET(MACRO_TILE_ASPECT,
macro_tile_aspect) |
> > + AMD_FMT_MOD_SET(NUM_BANKS, num_banks);
> > + fallthrough;
> > +
> > + case DC_ARRAY_1D_TILED_THIN1:
> > + /* Micro and macro tiled modes */
> > + modifier |=
> > + AMD_FMT_MOD |
> > + AMD_FMT_MOD_SET(TILE_VERSION,
AMD_FMT_MOD_TILE_VER_GFX6) |
> > + AMD_FMT_MOD_SET(TILE, array_mode) |
> > + AMD_FMT_MOD_SET(MICROTILE, micro_tile_mode);
> > + break;
> > +
> > + default:
> > + drm_err(&adev->ddev, "array mode 0x%x not supported by
DCE\n",
> > array_mode);
> [Severity: Medium]
> Could this drm_err() be triggered repeatedly by a user space process
> providing an invalid array_mode to DRM_IOCTL_MODE_ADDFB2, flooding the
> kernel log?
>
> Should this use a rate-limited print or drm_dbg_kms() instead to prevent log
> spam from unprivileged users?
I will just change this to drm_dbg_kms()
>
> > + return -EINVAL;
> > + }
> > +
> > + afb->base.modifier = modifier;
> > + afb->base.flags |= DRM_MODE_FB_MODIFIERS;
> > + return 0;
> > +}
>
> [ ... ]
>
> > @@ -1093,7 +1142,7 @@ static int amdgpu_display_verify_sizes(struct
> > amdgpu_framebuffer *rfb)>
> > get_block_dimensions(block_size_log2,
format_info->cpp[i],
> >
> > &block_width,
&block_height);
> >
> > - } else {
> > + } else if (AMD_FMT_MOD_GET(TILE_VERSION, modifier) >=
> > AMD_FMT_MOD_TILE_VER_GFX9) {>
> > int swizzle = AMD_FMT_MOD_GET(TILE,
modifier);
> >
> > switch ((swizzle & ~3) + 1) {
> >
> > @@ -1120,6 +1169,60 @@ static int amdgpu_display_verify_sizes(struct
> > amdgpu_framebuffer *rfb)>
> > get_block_dimensions(block_size_log2,
format_info->cpp[i],
> >
> > &block_width,
&block_height);
> >
> > + } else if (AMD_FMT_MOD_GET(TILE_VERSION, modifier) ==
> > AMD_FMT_MOD_TILE_VER_GFX6) { + const u32
display_micro_tile_pitch = 32;
> > /* required by DCE */ + const u32
micro_tile_width = 8;
> > + const u32 micro_tile_height = 8;
> > + const u32 micro_tile_mode =
AMD_FMT_MOD_GET(MICROTILE, modifier);
> > + const u32 array_mode = AMD_FMT_MOD_GET(TILE,
modifier);
> > + u32 num_banks, bank_width, bank_height,
pipe_config,
> > macro_tile_aspect;
> > + u32 num_pipes;
> > +
> > + if (AMD_FMT_MOD_GET(DCC, modifier)) {
> > + drm_dbg_kms(rfb->base.dev, "DCC
is not displayable on GFX6-8\n");
> > + return -EINVAL;
> > + }
> > + if (array_mode !=
AMD_FMT_MOD_TILE_GFX6_1D_TILED_THIN1 &&
> > + array_mode !=
AMD_FMT_MOD_TILE_GFX6_2D_TILED_THIN1) {
> > + drm_dbg_kms(rfb->base.dev,
> > + "Array mode %u is not
supported by the display driver\n",
> > + array_mode);
> > + return -EINVAL;
> > + }
> > + if (micro_tile_mode !=
AMD_FMT_MOD_MICROTILE_DISPLAY) {
> > + drm_dbg_kms(rfb->base.dev,
> > + "Micro tile mode %u is
not displayable on GFX6-8\n",
> > + micro_tile_mode);
> > + return -EINVAL;
> > + }
> > +
> > + num_banks = 2 << AMD_FMT_MOD_GET(NUM_BANKS,
modifier);
> > + bank_width = 1 <<
AMD_FMT_MOD_GET(BANK_WIDTH, modifier);
> > + bank_height = 1 <<
AMD_FMT_MOD_GET(BANK_HEIGHT, modifier);
> > + pipe_config = AMD_FMT_MOD_GET(PIPE_CONFIG,
modifier);
> > + macro_tile_aspect = 1 <<
AMD_FMT_MOD_GET(MACRO_TILE_ASPECT, modifier);
> > +
> > + if (pipe_config >=
AMD_FMT_MOD_PIPE_CONFIG_P16_32x32_8x16)
> > + num_pipes = 16;
> > + else if (pipe_config >=
AMD_FMT_MOD_PIPE_CONFIG_P8_16x16_8x16)
> > + num_pipes = 8;
> > + else if (pipe_config >=
AMD_FMT_MOD_PIPE_CONFIG_P4_8x16)
> > + num_pipes = 4;
> > + else if (pipe_config ==
AMD_FMT_MOD_PIPE_CONFIG_P2)
> > + num_pipes = 2;
> > + else
> > + unreachable();
>
> [Severity: High]
> Since pipe_config comes from AMD_FMT_MOD_GET(PIPE_CONFIG, modifier), which
> is user-controlled via the framebuffer modifier, could a user pass a value
> smaller than AMD_FMT_MOD_PIPE_CONFIG_P2?
>
> If so, hitting unreachable() here would result in undefined compiler
> behavior. With CONFIG_OBJTOOL enabled, it can emit an invalid instruction
> that immediately panics the kernel. If execution falls through, num_pipes
> remains uninitialized and is subsequently used in a multiplication.
I will replace unreachable() with "return -EINVAL"
>
> > +
> > + if (array_mode <
AMD_FMT_MOD_TILE_GFX6_2D_TILED_THIN1) {
> > + block_width =
display_micro_tile_pitch;
> > + block_height = micro_tile_height;
> > + } else {
> > + /* Assume non-PRT macro tiling
modes */
> > + block_width = num_pipes *
micro_tile_width *
> > + bank_width *
macro_tile_aspect;
> > + block_height = micro_tile_height
* bank_height *
> > + num_banks /
macro_tile_aspect;
> > + }
> >
> > }
> >
> > ret = amdgpu_display_verify_plane(rfb, i, format_info,
> >
> >
block_width, block_height, block_size_log2);
>
> [Severity: High]
> Does this pass an uninitialized block_size_log2 into
> amdgpu_display_verify_plane()?
I will fix that, thanks for noticing.
