Re: [PATCH 3/4] drm/amd/display: pass userspace brightness to power module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Double application of PWM bitmask quirk and mathematical corruption on millinits.

Pre-existing issues:
- [High] Sleeping function called from atomic context.
- [High] Potential division by zero in brightness conversion functions when processing malformed firmware capabilities.
--

commit 5d73b2399ca1fd59d638eb586a83e6e63cc7a221
Author: Andrei Rusu de Castro <arc@empyreal.works>

drm/amd/display: pass userspace brightness to power module

The power module consumes millipercent and builds its own PWM lookup table
from the ATIF brightness transfer characteristics. This patch passes the
original userspace percentage to the power module on PWM panels and keeps
the existing nits conversion and source-unit brightness mask for AUX panels.
It also wires the brightness bypass to the power module's existing linear
bypass.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index 424b33573a734..04e810af4623c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> @@ -218,6 +218,41 @@ u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
>  }
>  EXPORT_IF_KUNIT(convert_brightness_to_user);

[Severity: High]
This isn't a bug introduced by this patch, but can
convert_brightness_to_user() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c trigger a
kernel panic via division by zero?

If a connected monitor's EDID/DPCD reports identical max_luminance and
min_luminance values:
amdgpu_dm_update_backlight_caps() will set
aux_max_input_signal == aux_min_input_signal.

This causes max - min == 0 to be returned by get_brightness_range().
Subsequently, calling amdgpu_dm_backlight_get_level() will execute
convert_brightness_to_user(), which evaluates
DIV_ROUND_CLOSEST_ULL(..., max - min), triggering a division by zero.

This could allow a Denial of Service attack against the host system by
plugging in an untrusted, maliciously crafted DisplayPort monitor or dock.

> +static u32 convert_brightness_to_millipercent(const struct amdgpu_dm_backlight_caps *caps,
> +					      u32 brightness)
[ ... ]
> +STATIC_IFN_KUNIT
> +u32 convert_brightness_for_power_module(const struct amdgpu_dm_backlight_caps *caps,
> +					u32 user_brightness)
> +{
> +	u32 brightness;
> +
> +	if (!caps)
> +		return user_brightness;
> +
> +	if (!caps->aux_support)
> +		return convert_brightness_to_millipercent(caps, user_brightness);
> +
> +	brightness = convert_brightness_from_user(caps, user_brightness);
> +	if (caps->brightness_mask)
> +		brightness |= caps->brightness_mask;

[Severity: High]
Does convert_brightness_for_power_module() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c erroneously
apply the hardware bitmask quirk to a mathematical millinits value prior to
handoff?

For AUX panels, caps->brightness_mask is OR'ed into the mathematically
scaled millinits brightness variable here. This corrupted value is then
passed to mod_power_set_backlight_nits(), which converts it to a PWM value.

> +
> +	return brightness;
> +}
> +
> +EXPORT_IF_KUNIT(convert_brightness_for_power_module);
> +
>  STATIC_IFN_KUNIT
>  struct dc_stream_state *dm_find_stream_with_link(
>  	struct amdgpu_display_manager *dm,
> @@ -262,7 +297,6 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  	bool rc = false, reallow_idle = false;
>  	struct drm_connector *connector;
>  	struct dc_stream_state *stream;

[Severity: High]
This isn't a bug introduced by this patch, but does
amdgpu_dm_backlight_set_level() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c safely handle
calling mutex_lock(&dm->dc_lock)?

amdgpu_dm_backlight_set_level() explicitly locks the mutex, but it is
reached via amdgpu_dm_atomic_commit_tail() during non-blocking display
atomic commits. Calling a sleeping function from atomic context can lead to
kernel warnings (lockdep splats), system instability, and potential
deadlocks.

>  
>  	list_for_each_entry(connector, &dm->ddev->mode_config.connector_list, head) {
>  		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/modules/power/power_abm.c b/drivers/gpu/drm/amd/display/modules/power/power_abm.c
> index 5e86889eaa841..4d7d80ac60c9b 100644
> --- a/drivers/gpu/drm/amd/display/modules/power/power_abm.c
> +++ b/drivers/gpu/drm/amd/display/modules/power/power_abm.c
> @@ -1020,6 +1020,10 @@ void fill_backlight_level_params(struct core_power *core_power,
>  
>  	if (backlight_control_type == BACKLIGHT_CONTROL_AMD_AUX && !is_hdr)
>  		backlight_level_params->control_type = BACKLIGHT_CONTROL_PWM;
> +
> +	if (backlight_level_params->control_type == BACKLIGHT_CONTROL_PWM)
> +		backlight_level_params->backlight_pwm_u16_16 |=
> +			bl_prop->brightness_mask;

[Severity: High]
For non-HDR AUX panels falling back to PWM, does
fill_backlight_level_params() in
drivers/gpu/drm/amd/display/modules/power/power_abm.c apply the bitmask
a second time onto the resulting PWM value, leading to undefined hardware
behavior?

>  }
>  
>  bool mod_power_set_backlight_nits(struct mod_power *mod_power,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-brightness-cover-abf809f2@empyreal.works?part=3