Re: [PATCH 2/4] drm/amdgpu: Convert tiling flags to modifiers on GFX6-8

[sashiko-bot@kernel.org]

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 <timur.kristof@gmail.com>
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/20260715105757.13125-1-timur.kristof@gmail.com?part=2