Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing cpp factor in GFX6 block size calculation enables out-of-bounds VRAM read vulnerability. - [High] Strict rejection of RENDER microtiling introduces a functional regression for legacy GFX6 compositors. - [Low] Usage of undefined/DC-specific ASSERT macro in generic AMDGPU code causes build failures or macro misuse. --
--- Patch [2]: [PATCH 2/4] drm/amdgpu: Convert tiling flags to modifiers on GFX6-8 --- commit 44a2b5305f951bc68d3e2665bc5db9defc35dd24 Author: Timur Kristóf <[email protected]> Subject: drm/amdgpu: Convert tiling flags to modifiers on GFX6-8 This commit converts tiling flags to DRM format modifiers for AMD GFX8 and older GPUs, ensuring consistency across generations and allowing the display driver to rely on them. It includes logic to map array modes to modifiers and verify block sizes for GFX6 specifically. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c [ ... ] > + if (array_mode != AMD_FMT_MOD_TILE_GFX6_1D_TILED_THIN1 && > + array_mode != AMD_FMT_MOD_TILE_GFX6_2D_TILED_THIN1) { > + drm_dbg_kms(dev, "Array mode %u not supported by display > driver\n", array_mode); > + return -EINVAL; > + } > + if (micro_tile_mode != AMD_FMT_MOD_MICROTILE_DISPLAY) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this break legacy userspace compositors? Looking at check_tiling_flags_gfx6(), it explicitly supports RENDER microtiling (mode 3): micro_tile_mode = AMDGPU_TILING_GET(afb->tiling_flags, MICRO_TILE_MODE); switch (micro_tile_mode) { case 0: /* DISPLAY */ case 3: /* RENDER */ return 0; Could returning -EINVAL here for RENDER mode cause a regression for legacy clients allocating display buffers for 3D performance? > + drm_dbg_kms(dev, "Microtile mode %u not displayable\n", > micro_tile_mode); > + return -EINVAL; > + } [ ... ] > @@ -1120,6 +1247,15 @@ 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) { > + ret = amdgpu_display_verify_sizes_gfx6(rfb->base.dev, > modifier, > + &block_width, > &block_height); > + if (ret) > + return ret; > + > + ASSERT(is_power_of_2(block_width)); ^^^^^^ [Severity: Low] Are these ASSERT() macros defined in this context? ASSERT is typically a Display Core (DC) specific macro (from dc/os_types.h). Since amdgpu_display.c is part of the core driver, would WARN_ON() be more appropriate here to avoid potential build failures? > + ASSERT(is_power_of_2(block_height)); > + block_size_log2 = ilog2(block_width * block_height); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Should this calculation include the bytes-per-pixel factor? By omitting the format_info->cpp[i] multiplier, block_size_log2 appears to underestimate the block size. When this is later passed into amdgpu_display_verify_plane(), it calculates: unsigned int block_size = 1 << block_size_log2; ... size = rfb->base.offsets[plane] + (uint64_t)rfb->base.pitches[plane] / block_pitch * block_size * DIV_ROUND_UP(height, block_height); if (rfb->base.obj[0]->size < size) { An underestimated block_size results in a vastly smaller size requirement, which might allow an undersized Buffer Object to pass validation, potentially leading to out-of-bounds VRAM reads. > } > > ret = amdgpu_display_verify_plane(rfb, i, format_info, -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
