Re: [PATCH 06/12] drm/panic: Pass global module parameters to drm_panic_dispatch()

Am 18.08.26 um 15:09 schrieb [email protected]:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Concurrent debugfs writes or KUnit tests across multiple DRM devices race on global static buffers without adequate synchronization.

A fix would be nice, but is beyond this series. As it affects debugfs 
and kunit test there's no urgency.

> --
>
> --- Patch [6]: [PATCH 06/12] drm/panic: Pass global module parameters to drm_panic_dispatch() ---
> commit 9f2a1b5158feac614c09a17c83f387591642e034
> Author: Thomas Zimmermann <[email protected]>
>
> drm/panic: Pass global module parameters to drm_panic_dispatch()
>
> This patch sets the panic type and QR version in draw_panic_plane() as a single
> point of truth, passing these values into the individual drawing functions.
> In the test case, it always creates a QR code with version 40.
>
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> index ede620b27515a..12c39217cbcb5 100644
>> --- a/drivers/gpu/drm/drm_panic.c
>> +++ b/drivers/gpu/drm/drm_panic.c
> [ ... ]
>> @@ -951,6 +955,11 @@ static void draw_panic_plane(struct drm_plane *plane, const char *description)
>>   #else
>>   	u32 bg_color = 0x00000000;
>>   #endif
>> +#if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
>> +	unsigned int qr_version = panic_qr_version;
>> +#else
>> +	unsigned int qr_version = 0;
>> +#endif
>>   
>>   	if (!drm_panic_trylock(plane->dev, flags))
>>   		return;
>> @@ -966,7 +975,7 @@ static void draw_panic_plane(struct drm_plane *plane, const char *description)
>>   
>>   	drm_panic_set_description(description);
>>   
>> -	ret = draw_panic_dispatch(&sb, fg_color, bg_color);
>> +	ret = draw_panic_dispatch(&sb, drm_panic_type, fg_color, bg_color, qr_version);
> [Severity: High]
> This is a pre-existing issue, but does the per-device locking in
> drm_panic_trylock() adequately protect the global buffers?
>
> Since the lock is per-device (plane->dev->mode_config.panic_lock), concurrent
> debugfs writes across multiple DRM devices can still race and mutate global
> static variables, such as the zlib stream workspace (struct z_stream_s stream)
> or qrbuf1 and qrbuf2.
>
> If a real kernel panic occurs while a debugfs write is concurrently using these
> buffers, the true panic handler could inherit a corrupted zlib workspace state,
> which might lead to a recursive fault and failure to display the panic screen.
>
> Could this be protected by a global lock for non-panic paths?
>
> [ ... ]
>> diff --git a/drivers/gpu/drm/tests/drm_panic_test.c b/drivers/gpu/drm/tests/drm_panic_test.c
>> index 969a096ec840f..196496238d231 100644
>> --- a/drivers/gpu/drm/tests/drm_panic_test.c
>> +++ b/drivers/gpu/drm/tests/drm_panic_test.c
>> @@ -58,7 +58,7 @@ static int draw_panic_screen_kmsg_default(struct drm_scanout_buffer *sb)
>>   #if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
>>   static int draw_panic_screen_qr_code_default(struct drm_scanout_buffer *sb)
>>   {
>> -	return draw_panic_screen_qr_code(sb, 0x00ffffff, 0x00000000);
>> +	return draw_panic_screen_qr_code(sb, 0x00ffffff, 0x00000000, 40);
>>   }
>>   #endif
> [Severity: High]
> This isn't a bug introduced by this patch, but do the KUnit tests safely access
> the global drawing state?
>
> It appears the KUnit tests bypass draw_panic_plane() entirely and directly call
> drawing functions like draw_panic_screen_qr_code() without acquiring any locks.
> Could running the tests race on the global buffers and corrupt the zlib
> workspace?
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)