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

["Timur Kristóf" <timur.kristof@gmail.com>]

On 2026. július 15., szerda 13:10:51 közép-európai nyári idő sashiko-
bot@kernel.org 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 <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?

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.