Re: [PATCH AUTOSEL 6.18-5.10] drm/amdgpu: Validate ATIF buffer length before use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: unvalidated number_of_points in amdgpu_atif_query_backlight_caps()
--

commit a73a09a7b2fb3e5ffd26ffc3c59fb52e0414b2a7
Author: Lijo Lazar <lijo.lazar@amd.com>

drm/amdgpu: Validate ATIF buffer length before use

This commit adds a min_size parameter to amdgpu_atif_call() to validate that
the returned ACPI buffer is of type ACPI_TYPE_BUFFER, holds at least a u16
size field, does not claim more data than was actually returned, and meets
the minimum size required by each calling function.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 6c62e27b98002..5be40c781807a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
[ ... ]
> @@ -371,20 +383,14 @@ static int amdgpu_atif_query_backlight_caps(struct amdgpu_atif *atif)
>  
>  	info = amdgpu_atif_call(atif,
>  		ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS,
> -		&params);
> +		&params, offsetof(struct atif_qbtc_output, data_points));
>  	if (!info) {
>  		err = -EIO;
>  		goto out;
>  	}
>  
> -	size = *(u16 *) info->buffer.pointer;
> -	if (size < 10) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
>  	memset(&characteristics, 0, sizeof(characteristics));
> -	size = min(sizeof(characteristics), size);
> +	size = min(sizeof(characteristics), (size_t)*(u16 *)info->buffer.pointer);
>  	memcpy(&characteristics, info->buffer.pointer, size);
>  
>  	atif->backlight_caps.caps_valid = true;

[Severity: High]
Should amdgpu_atif_query_backlight_caps() also validate
characteristics.number_of_points against ATIF_QBTC_MAX_DATA_POINTS and the
returned buffer size?

Right after this hunk in drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c,
amdgpu_atif_query_backlight_caps() stores characteristics.number_of_points (a
u8) directly into atif->backlight_caps.data_points:

	atif->backlight_caps.data_points = characteristics.number_of_points;
	memcpy(atif->backlight_caps.luminance_data,
	       characteristics.data_points,
	       sizeof(atif->backlight_caps.luminance_data));

Because amdgpu_atif_call() only validates that the buffer is at least
offsetof(struct atif_qbtc_output, data_points) bytes, a malformed ATIF
response can report characteristics.number_of_points greater than
ATIF_QBTC_MAX_DATA_POINTS (99), or report more data points than are actually
present in the returned buffer size.

When backlight brightness is later updated via:

  amdgpu_dm_backlight_update_status() ->
    amdgpu_dm_backlight_set_level() ->
      convert_brightness_from_user() ->
        convert_custom_brightness()

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:convert_custom_brightness()
uses caps->data_points to index caps->luminance_data[MAX_LUMINANCE_DATA_POINTS]
without clamping:

	left = 0;
	right = caps->data_points - 1;
	while (left <= right) {
		int mid = left + (right - left) / 2;
		u8 signal = caps->luminance_data[mid].input_signal;

Could this read out of bounds of caps->luminance_data[] when
characteristics.number_of_points exceeds ATIF_QBTC_MAX_DATA_POINTS, or read
zero-padded entries if size is smaller than
offsetof(struct atif_qbtc_output, data_points) +
characteristics.number_of_points * sizeof(struct atif_qbtc_data_point)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-55-sashal@kernel.org?part=1