linux-kernel - Re: [PATCH] drm/i915/gvt: Add __nonstring annotations for unterminated strings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r02ma8s3.fsf@intel.com>
Date: Mon, 24 Mar 2025 14:54:36 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Damian Tometzki <damian@...cv-rocks.de>, Kees Cook <kees@...nel.org>
Cc: Zhenyu Wang <zhenyuw@...ux.intel.com>, Zhi Wang
 <zhi.wang.linux@...il.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
 Tvrtko Ursulin <tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, intel-gvt-dev@...ts.freedesktop.org,
 intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] drm/i915/gvt: Add __nonstring annotations for
 unterminated strings

On Sun, 23 Mar 2025, Damian Tometzki <damian@...cv-rocks.de> wrote:
> On Mon, 10. Mar 15:23, Kees Cook wrote:
>> When a character array without a terminating NUL character has a static
>> initializer, GCC 15's -Wunterminated-string-initialization will only
>> warn if the array lacks the "nonstring" attribute[1]. Mark the arrays
>> with __nonstring to and correctly identify the char array as "not a C
>> string" and thereby eliminate the warning.
>> 
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
>> Cc: Zhenyu Wang <zhenyuw@...ux.intel.com>
>> Cc: Zhi Wang <zhi.wang.linux@...il.com>
>> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@...el.com>
>> Cc: Tvrtko Ursulin <tursulin@...ulin.net>
>> Cc: David Airlie <airlied@...il.com>
>> Cc: Simona Vetter <simona@...ll.ch>
>> Cc: intel-gvt-dev@...ts.freedesktop.org
>> Cc: intel-gfx@...ts.freedesktop.org
>> Cc: dri-devel@...ts.freedesktop.org
>> Signed-off-by: Kees Cook <kees@...nel.org>
>> ---
>>  drivers/gpu/drm/i915/gvt/opregion.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
>> index 509f9ccae3a9..f701638d3145 100644
>> --- a/drivers/gpu/drm/i915/gvt/opregion.c
>> +++ b/drivers/gpu/drm/i915/gvt/opregion.c
>> @@ -43,7 +43,7 @@
>>  #define DEVICE_TYPE_EFP4   0x10
>>  
>>  struct opregion_header {
>> -	u8 signature[16];
>> +	u8 signature[16] __nonstring;

Why would this annotation be needed? It's not treated as a string
anywhere, and it's u8 not char.

>>  	u32 size;
>>  	u32 opregion_ver;
>>  	u8 bios_ver[32];
>> @@ -222,7 +222,7 @@ int intel_vgpu_init_opregion(struct intel_vgpu *vgpu)
>>  	u8 *buf;
>>  	struct opregion_header *header;
>>  	struct vbt v;
>> -	const char opregion_signature[16] = OPREGION_SIGNATURE;
>> +	const char opregion_signature[16] __nonstring = OPREGION_SIGNATURE;
>>  
>>  	gvt_dbg_core("init vgpu%d opregion\n", vgpu->id);
>>  	vgpu_opregion(vgpu)->va = (void *)__get_free_pages(GFP_KERNEL |
>> -- 
>> 2.34.1
>> 
> Hello together,
>
> it doesnt resolve the build issue with gcc15 gcc (GCC) 15.0.1 20250228
>
> CC [M]  drivers/gpu/drm/i915/gvt/scheduler.o
> /home/damian/kernel/linux/drivers/gpu/drm/i915/gvt/opregion.c: In function ‘intel_vgpu_init_opregion’:
> /home/damian/kernel/linux/drivers/gpu/drm/i915/gvt/opregion.c:35:28: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>    35 | #define OPREGION_SIGNATURE "IntelGraphicsMem"
>       |                            ^~~~~~~~~~~~~~~~~~
> /home/damian/kernel/linux/drivers/gpu/drm/i915/gvt/opregion.c:225:57: note: in expansion of macro ‘OPREGION_SIGNATURE’
>   225 |         const char opregion_signature[16] __nonstring = OPREGION_SIGNATURE;
>       |                                                         ^~~~~~~~~~~~~~~~~~
>   CC [M]  drivers/gpu/drm/i915/gvt/trace_points.o
> cc1: all warnings being treated as errors
> make[7]: *** [/home/damian/kernel/linux/scripts/Makefile.build:207: drivers/gpu/drm/i915/gvt/opregion.o] Error 1
> make[7]: *** Waiting for unfinished jobs....
>   CC [M]  drivers/gpu/drm/i915/gvt/vgpu.o
> make[6]: *** [/home/damian/kernel/linux/scripts/Makefile.build:465: drivers/gpu/drm/i915] Error 2
> make[5]: *** [/home/damian/kernel/linux/s

What about this?

IMO it's anyway good practice to use sizeof(dest) rather than
sizeof(src) for memcpy.


diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 509f9ccae3a9..dbad4d853d3a 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -222,7 +222,6 @@ int intel_vgpu_init_opregion(struct intel_vgpu *vgpu)
 	u8 *buf;
 	struct opregion_header *header;
 	struct vbt v;
-	const char opregion_signature[16] = OPREGION_SIGNATURE;
 
 	gvt_dbg_core("init vgpu%d opregion\n", vgpu->id);
 	vgpu_opregion(vgpu)->va = (void *)__get_free_pages(GFP_KERNEL |
@@ -236,8 +235,10 @@ int intel_vgpu_init_opregion(struct intel_vgpu *vgpu)
 	/* emulated opregion with VBT mailbox only */
 	buf = (u8 *)vgpu_opregion(vgpu)->va;
 	header = (struct opregion_header *)buf;
-	memcpy(header->signature, opregion_signature,
-	       sizeof(opregion_signature));
+
+	static_assert(sizeof(header->signature) == sizeof(OPREGION_SIGNATURE) - 1);
+	memcpy(header->signature, OPREGION_SIGNATURE, sizeof(header->signature));
+
 	header->size = 0x8;
 	header->opregion_ver = 0x02000000;
 	header->mboxes = MBOX_VBT;



-- 
Jani Nikula, Intel