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 <[email protected]> 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, > - ¶ms); > + ¶ms, 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/[email protected]?part=1
