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

[Vignesh Raman <vignesh.raman@collabora.com>]

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