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? > + 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. > + > + 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()? It appears block_size_log2 is never set in the new AMD_FMT_MOD_TILE_VER_GFX6 branch just above this call. Inside amdgpu_display_verify_plane(), this uninitialized value is used in a shift: unsigned int block_size = 1 << block_size_log2; If the uninitialized stack garbage causes block_size to evaluate to 0, this will trigger a divide-by-zero panic on the subsequent modulo check: if (rfb->base.offsets[plane] % block_size) Or if it underestimates the BO size, could this lead to out-of-bounds VRAM access during scanout? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
