[igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Vignesh Raman]

In crosvm, the kernel reports 16 for count_crtcs, which exceeds
IGT_MAX_PIPES set to 8. The function igt_display_require allocates
memory for IGT_MAX_PIPES members of igt_pipe_t structures, but then
writes into it based on the count_crtcs reported by the kernel,
resulting in memory corruption.

 # malloc(): corrupted top size
 # Received signal SIGABRT.
 # Stack trace:
 #  #0 [fatal_sig_handler+0x17b]
 #  #1 [__sigaction+0x40]
 #  #2 [pthread_key_delete+0x14c]
 #  #3 [gsignal+0x12]
 #  #4 [abort+0xd3]
 #  #5 [__fsetlocking+0x290]
 #  #6 [timer_settime+0x37a]
 #  #7 [__default_morecore+0x1f1b]
 #  #8 [__libc_calloc+0x161]
 #  #9 [drmModeGetPlaneResources+0x44]
 #  #10 [igt_display_require+0x194]
 #  #11 [__igt_unique____real_main1356+0x93c]
 #  #12 [main+0x3f]
 #  #13 [__libc_init_first+0x8a]
 #  #14 [__libc_start_main+0x85]
 #  #15 [_start+0x21]

Increase IGT_MAX_PIPES to 16 to fix this memory corruption issue.
This fix is required for drm-ci to run igt tests on virtio-gpu.

Suggested-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
Signed-off-by: Vignesh Raman <vignesh.raman@collabora.com>
---

v2:
  - Rework the fix to increase IGT_MAX_PIPES to 16

---
 lib/igt_kms.c |  2 +-
 lib/igt_kms.h | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 453103f90..9239b71c4 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -906,7 +906,7 @@ static igt_plane_t *igt_get_assigned_primary(igt_output_t *output, igt_pipe_t *p
  */
 const char *kmstest_pipe_name(enum pipe pipe)
 {
-	static const char str[] = "A\0B\0C\0D\0E\0F\0G\0H";
+	static const char str[] = "A\0B\0C\0D\0E\0F\0G\0H\0I\0J\0K\0L\0M\0N\0O\0P";
 
 	_Static_assert(sizeof(str) == IGT_MAX_PIPES * 2,
 		       "Missing pipe name");
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 9028ab9be..5c705b585 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -57,6 +57,16 @@
  * @PIPE_D: Fourth crtc.
  * @PIPE_E: Fifth crtc.
  * @PIPE_F: Sixth crtc.
+ * @PIPE_G: Seventh crtc.
+ * @PIPE_H: Eighth crtc.
+ * @PIPE_I: Ninth crtc.
+ * @PIPE_J: Tenth crtc.
+ * @PIPE_K: Eleventh crtc.
+ * @PIPE_L: Twelfth crtc.
+ * @PIPE_M: Thirteenth crtc.
+ * @PIPE_N: Fourteenth crtc.
+ * @PIPE_O: Fifteenth crtc.
+ * @PIPE_P: Sixteenth crtc.
  * @IGT_MAX_PIPES: Max number of pipes allowed.
  */
 enum pipe {
@@ -70,7 +80,15 @@ enum pipe {
         PIPE_F,
 	PIPE_G,
 	PIPE_H,
-        IGT_MAX_PIPES
+	PIPE_I,
+	PIPE_J,
+	PIPE_K,
+	PIPE_L,
+	PIPE_M,
+	PIPE_N,
+	PIPE_O,
+	PIPE_P,
+	IGT_MAX_PIPES
 };
 const char *kmstest_pipe_name(enum pipe pipe);
 int kmstest_pipe_to_index(char pipe);
-- 
2.40.1

Re: [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Daniel Stone]

Hi Vignesh,

On 27/10/2023 15:40, Vignesh Raman wrote:
> In crosvm, the kernel reports 16 for count_crtcs, which exceeds
> IGT_MAX_PIPES set to 8. The function igt_display_require allocates
> memory for IGT_MAX_PIPES members of igt_pipe_t structures, but then
> writes into it based on the count_crtcs reported by the kernel,
> resulting in memory corruption.

To make this robust against future changes (32 CRTCs seems totally 
fanciful, but so did 16 a while ago), this needs to also be robust 
against count_crtcs exceeding IGT_NUM_PIPES, along the lines of your 
previous change.

Cheers, Daniel

[igt-dev] ✗ Fi.CI.BUILD: failure for lib/igt_kms: Fix memory corruption (rev2)

[Patchwork]

== Series Details ==

Series: lib/igt_kms: Fix memory corruption (rev2)
URL   : https://patchwork.freedesktop.org/series/125599/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
5e82a8ee42d58c5e183c3d4208ae4ccd977a4830 intel-ci/xe-fast-feedback: Add two xe_evict_ccs subtests for Xe run

Tail of build.log:
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-basic-y-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-basic-y-tiled-gen12-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-basic-y-tiled-gen12-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-basic-y-tiled-gen12-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-basic-yf-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-dg2-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-dg2-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-dg2-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-mtl-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-mtl-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-4-tiled-mtl-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-y-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-y-tiled-gen12-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-y-tiled-gen12-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-y-tiled-gen12-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-primary-rotation-180-yf-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-dg2-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-dg2-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-dg2-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-mtl-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-mtl-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-4-tiled-mtl-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-y-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-y-tiled-gen12-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-y-tiled-gen12-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-y-tiled-gen12-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-crc-sprite-planes-basic-yf-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-4-tiled-mtl-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-4-tiled-mtl-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-4-tiled-mtl-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-y-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-y-tiled-gen12-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-y-tiled-gen12-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-y-tiled-gen12-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
Please refer: docs/test_documentation.md for more details
[1648/1651] Generating xe_tests.html with a custom command.
[1649/1651] Generating i915_tests.html with a custom command.
ninja: build stopped: subcommand failed.

[igt-dev] ✗ GitLab.Pipeline: warning for lib/igt_kms: Fix memory corruption (rev2)

[Patchwork]

== Series Details ==

Series: lib/igt_kms: Fix memory corruption (rev2)
URL   : https://patchwork.freedesktop.org/series/125599/
State : warning

== Summary ==

Pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1020725 for the overview.

build:tests-debian-meson has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889720):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698430990:step_script
  section_start:1698430990:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698430992:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889715):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698430994:step_script
  section_start:1698430994:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698430995:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-clang has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889719):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698431011:step_script
  section_start:1698431011:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698431012:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-no-libdrm-nouveau has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889718):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698430990:step_script
  section_start:1698430990:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698430992:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-no-libunwind has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889716):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698430993:step_script
  section_start:1698430993:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698430994:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-oldest-meson has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/50889717):
  Warning: Missing documentation for igt@kms_ccs@pipe-P-missing-ccs-buffer-yf-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-dg2-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-4-tiled-mtl-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-mc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-y-tiled-gen12-rc-ccs-cc
  Warning: Missing documentation for igt@kms_ccs@pipe-P-random-ccs-data-yf-tiled-ccs
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1698430991:step_script
  section_start:1698430991:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1698430992:cleanup_file_variables
  ERROR: Job failed: exit code 1

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1020725

Re: [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Vignesh Raman]

Hi Daniel,

On 27/10/23 22:07, Daniel Stone wrote:
> Hi Vignesh,
> 
> On 27/10/2023 15:40, Vignesh Raman wrote:
>> In crosvm, the kernel reports 16 for count_crtcs, which exceeds
>> IGT_MAX_PIPES set to 8. The function igt_display_require allocates
>> memory for IGT_MAX_PIPES members of igt_pipe_t structures, but then
>> writes into it based on the count_crtcs reported by the kernel,
>> resulting in memory corruption.
> 
> To make this robust against future changes (32 CRTCs seems totally 
> fanciful, but so did 16 a while ago), this needs to also be robust 
> against count_crtcs exceeding IGT_NUM_PIPES, along the lines of your 
> previous change.

Yes, I agree to make the code robust against potential changes in the 
future. We can set IGT_MAX_PIPES to 32. I will send an updated patch.

Thanks.

Regards,
Vignesh

Re: [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Daniel Stone]

Hi Vignesh,

On 06/11/2023 02:40, Vignesh Raman wrote:
> Hi Daniel,
>
> On 27/10/23 22:07, Daniel Stone wrote:
>> Hi Vignesh,
>>
>> On 27/10/2023 15:40, Vignesh Raman wrote:
>>> In crosvm, the kernel reports 16 for count_crtcs, which exceeds
>>> IGT_MAX_PIPES set to 8. The function igt_display_require allocates
>>> memory for IGT_MAX_PIPES members of igt_pipe_t structures, but then
>>> writes into it based on the count_crtcs reported by the kernel,
>>> resulting in memory corruption.
>>
>> To make this robust against future changes (32 CRTCs seems totally 
>> fanciful, but so did 16 a while ago), this needs to also be robust 
>> against count_crtcs exceeding IGT_NUM_PIPES, along the lines of your 
>> previous change.
>
> Yes, I agree to make the code robust against potential changes in the 
> future. We can set IGT_MAX_PIPES to 32. I will send an updated patch.

What I mean is to make sure that the code cannot access out of bounds, 
regardless of the number. We can set IGT_MAX_PIPES to 32 but then 
someone will hit the same failure when there are more CRTCs than that.

The way to do this would probably be to igt_require() that count_crtcs 
does not exceed IGT_MAX_PIPES, in all the paths where we access the 
crtcs array. There is no need to bump the limit to 32 until it's required.

Cheers,
Daniel

Re: [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Vignesh Raman]

Hi Daniel,

On 06/11/23 20:13, Daniel Stone wrote:
> Hi Vignesh,
> 
> On 06/11/2023 02:40, Vignesh Raman wrote:
>> Hi Daniel,
>>
>> On 27/10/23 22:07, Daniel Stone wrote:
>>> Hi Vignesh,
>>>
>>> On 27/10/2023 15:40, Vignesh Raman wrote:
>>>> In crosvm, the kernel reports 16 for count_crtcs, which exceeds
>>>> IGT_MAX_PIPES set to 8. The function igt_display_require allocates
>>>> memory for IGT_MAX_PIPES members of igt_pipe_t structures, but then
>>>> writes into it based on the count_crtcs reported by the kernel,
>>>> resulting in memory corruption.
>>>
>>> To make this robust against future changes (32 CRTCs seems totally 
>>> fanciful, but so did 16 a while ago), this needs to also be robust 
>>> against count_crtcs exceeding IGT_NUM_PIPES, along the lines of your 
>>> previous change.
>>
>> Yes, I agree to make the code robust against potential changes in the 
>> future. We can set IGT_MAX_PIPES to 32. I will send an updated patch.
> 
> What I mean is to make sure that the code cannot access out of bounds, 
> regardless of the number. We can set IGT_MAX_PIPES to 32 but then 
> someone will hit the same failure when there are more CRTCs than that.
> 
> The way to do this would probably be to igt_require() that count_crtcs 
> does not exceed IGT_MAX_PIPES, in all the paths where we access the 
> crtcs array. There is no need to bump the limit to 32 until it's required.
> 

Okay understood. We can use igt_assert_f to make the test fail (instead 
of skipping it) if count_crtcs exceeds IGT_MAX_PIPES.

ERROR - Igt error: (kms_vblank:81) igt_kms-CRITICAL: Failed assertion: 
resources->count_crtcs <= IGT_MAX_PIPES
ERROR - Igt error: (kms_vblank:81) igt_kms-CRITICAL: count_crtcs exceeds 
IGT_MAX_PIPES, count_crtcs=16, IGT_MAX_PIPES=8
ERROR - Igt error: Test kms_vblank failed.

I will check other code paths as well. Thanks.

Regards,
Vignesh

Re: [igt-dev] [PATCH i-g-t v2] lib/igt_kms: Fix memory corruption

[Daniel Stone]

On 07/11/2023 08:58, Vignesh Raman wrote:
> On 06/11/23 20:13, Daniel Stone wrote:
>> What I mean is to make sure that the code cannot access out of 
>> bounds, regardless of the number. We can set IGT_MAX_PIPES to 32 but 
>> then someone will hit the same failure when there are more CRTCs than 
>> that.
>>
>> The way to do this would probably be to igt_require() that 
>> count_crtcs does not exceed IGT_MAX_PIPES, in all the paths where we 
>> access the crtcs array. There is no need to bump the limit to 32 
>> until it's required.
>
> Okay understood. We can use igt_assert_f to make the test fail 
> (instead of skipping it) if count_crtcs exceeds IGT_MAX_PIPES.
>
> ERROR - Igt error: (kms_vblank:81) igt_kms-CRITICAL: Failed assertion: 
> resources->count_crtcs <= IGT_MAX_PIPES
> ERROR - Igt error: (kms_vblank:81) igt_kms-CRITICAL: count_crtcs 
> exceeds IGT_MAX_PIPES, count_crtcs=16, IGT_MAX_PIPES=8
> ERROR - Igt error: Test kms_vblank failed.
>
> I will check other code paths as well. Thanks.

That's perfect, thanks Vignesh.

Cheers,

Daniel