[PATCH 1/3] Revert "drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]"

[PATCH 1/3] Revert "drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]"

[Nicolas Chauvet]

This reverts commit ea26c96d59b27e878fe61e8ef0fed840d2281a2f.

This fix truncates the OPREGION_SIGNATURE to fit into 16 chars instead of
enlarging the target field, hence only moving the size missmatch to later.

As shown with gcc-15:
drivers/gpu/drm/i915/gvt/opregion.c: In function intel_vgpu_init_opregion:
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"
      |                            ^~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/gvt/opregion.c:225:45: note: in expansion of macro OPREGION_SIGNATURE
  225 |         const char opregion_signature[16] = OPREGION_SIGNATURE;
      |                                             ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Cc: stable@vger.kernel.org
Reported-by: Nicolas Chauvet <kwizart@gmail.com>
Fixes: ea26c96d59 ("drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 drivers/gpu/drm/i915/gvt/opregion.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 509f9ccae3a9..9a8ead6039e2 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,8 @@ 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));
+	memcpy(header->signature, OPREGION_SIGNATURE,
+			sizeof(OPREGION_SIGNATURE));
 	header->size = 0x8;
 	header->opregion_ver = 0x02000000;
 	header->mboxes = MBOX_VBT;
-- 
2.49.0

Re: [PATCH 1/3] Revert "drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]"

[Jani Nikula]

On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
> This reverts commit ea26c96d59b27e878fe61e8ef0fed840d2281a2f.
>
> This fix truncates the OPREGION_SIGNATURE to fit into 16 chars instead of
> enlarging the target field, hence only moving the size missmatch to later.
>
> As shown with gcc-15:
> drivers/gpu/drm/i915/gvt/opregion.c: In function intel_vgpu_init_opregion:
> 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"
>       |                            ^~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/gvt/opregion.c:225:45: note: in expansion of macro OPREGION_SIGNATURE
>   225 |         const char opregion_signature[16] = OPREGION_SIGNATURE;
>       |                                             ^~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Cc: stable@vger.kernel.org
> Reported-by: Nicolas Chauvet <kwizart@gmail.com>
> Fixes: ea26c96d59 ("drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>

This introduces a buffer overflow.

sizeof(OPREGION_SIGNATURE) == 17.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/gvt/opregion.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
> index 509f9ccae3a9..9a8ead6039e2 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,8 @@ 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));
> +	memcpy(header->signature, OPREGION_SIGNATURE,
> +			sizeof(OPREGION_SIGNATURE));
>  	header->size = 0x8;
>  	header->opregion_ver = 0x02000000;
>  	header->mboxes = MBOX_VBT;

-- 
Jani Nikula, Intel

[PATCH 2/3] [RFC] drm/i915/gvt: Fix opregion_header->signature size

[Nicolas Chauvet]

Enlarge the signature field to accept the string termination.

Cc: stable@vger.kernel.org
Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 9a8ead6039e2..0f11cd6ba383 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[32];
 	u32 size;
 	u32 opregion_ver;
 	u8 bios_ver[32];
-- 
2.49.0

Re: [PATCH 2/3] [RFC] drm/i915/gvt: Fix opregion_header->signature size

[Jani Nikula]

On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
> Enlarge the signature field to accept the string termination.
>
> Cc: stable@vger.kernel.org
> Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>

Nope, can't do that. The packed struct is used for parsing data in
memory.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
> index 9a8ead6039e2..0f11cd6ba383 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[32];
>  	u32 size;
>  	u32 opregion_ver;
>  	u8 bios_ver[32];

-- 
Jani Nikula, Intel

Re: [PATCH 2/3] [RFC] drm/i915/gvt: Fix opregion_header->signature size

[Jani Nikula]

On Mon, 24 Mar 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
>> Enlarge the signature field to accept the string termination.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
>> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
>
> Nope, can't do that. The packed struct is used for parsing data in
> memory.

Okay, so I mixed this up with display/intel_opregion.c. So it's not used
for parsing here... but it's used for generating the data in memory, and
we can't change the layout or contents.

Regardless, we can't do either patch 2 or patch 3.

BR,
Jani.


>
> BR,
> Jani.
>
>
>> ---
>>  drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
>> index 9a8ead6039e2..0f11cd6ba383 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[32];
>>  	u32 size;
>>  	u32 opregion_ver;
>>  	u8 bios_ver[32];

-- 
Jani Nikula, Intel

Re: [PATCH 2/3] [RFC] drm/i915/gvt: Fix opregion_header->signature size

[Nicolas Chauvet]

Le lun. 24 mars 2025 à 10:34, Jani Nikula
<jani.nikula@linux.intel.com> a écrit :
>
> On Mon, 24 Mar 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
> >> Enlarge the signature field to accept the string termination.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
> >> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
> >
> > Nope, can't do that. The packed struct is used for parsing data in
> > memory.
>
> Okay, so I mixed this up with display/intel_opregion.c. So it's not used
> for parsing here... but it's used for generating the data in memory, and
> we can't change the layout or contents.
>
> Regardless, we can't do either patch 2 or patch 3.

Thanks for review.
So does it means the only "Fix" is to drop Werror, at least for intel/gvt code ?
I have CONFIG_DRM_I915_WERROR not set but CONFIG_DRM_WERROR=y, (same as Fedora)
Unsure why the current Fedora kernel is unaffected by this build failure.

Re: [PATCH 2/3] [RFC] drm/i915/gvt: Fix opregion_header->signature size

[Jani Nikula]

On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
> Le lun. 24 mars 2025 à 10:34, Jani Nikula
> <jani.nikula@linux.intel.com> a écrit :
>>
>> On Mon, 24 Mar 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
>> >> Enlarge the signature field to accept the string termination.
>> >>
>> >> Cc: stable@vger.kernel.org
>> >> Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
>> >> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
>> >
>> > Nope, can't do that. The packed struct is used for parsing data in
>> > memory.
>>
>> Okay, so I mixed this up with display/intel_opregion.c. So it's not used
>> for parsing here... but it's used for generating the data in memory, and
>> we can't change the layout or contents.
>>
>> Regardless, we can't do either patch 2 or patch 3.
>
> Thanks for review.
> So does it means the only "Fix" is to drop Werror, at least for intel/gvt code ?

Of course not. The fix is to address the warning.

There's another thread about this, see my suggestion there [1].

BR,
Jani.


[1] https://lore.kernel.org/r/87r02ma8s3.fsf@intel.com


> I have CONFIG_DRM_I915_WERROR not set but CONFIG_DRM_WERROR=y, (same as Fedora)
> Unsure why the current Fedora kernel is unaffected by this build failure.

-- 
Jani Nikula, Intel

[PATCH 3/3] [RFC] drm/i915/gvt: change OPREGION_SIGNATURE name

[Nicolas Chauvet]

Change the OPREGION_SIGNATURE name so it fit into the
opregion_header->signature size.

Cc: stable@vger.kernel.org
Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 0f11cd6ba383..0bd02dfaceb1 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -32,7 +32,7 @@
 #define _INTEL_BIOS_PRIVATE
 #include "display/intel_vbt_defs.h"
 
-#define OPREGION_SIGNATURE "IntelGraphicsMem"
+#define OPREGION_SIGNATURE "IntelGFXMem"
 #define MBOX_VBT      (1<<3)
 
 /* device handle */
-- 
2.49.0

Re: [PATCH 3/3] [RFC] drm/i915/gvt: change OPREGION_SIGNATURE name

[Jani Nikula]

On Mon, 24 Mar 2025, Nicolas Chauvet <kwizart@gmail.com> wrote:
> Change the OPREGION_SIGNATURE name so it fit into the
> opregion_header->signature size.
>
> Cc: stable@vger.kernel.org
> Fixes: 93615d59912 ("Revert drm/i915/gvt: Fix out-of-bounds buffer write into opregion->signature[]")
> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>

Nope, can't do that. The signature is used for checking data in
memory. Which should be obvious if you'd looked at how it's being used
or if this was tested.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
> index 0f11cd6ba383..0bd02dfaceb1 100644
> --- a/drivers/gpu/drm/i915/gvt/opregion.c
> +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> @@ -32,7 +32,7 @@
>  #define _INTEL_BIOS_PRIVATE
>  #include "display/intel_vbt_defs.h"
>  
> -#define OPREGION_SIGNATURE "IntelGraphicsMem"
> +#define OPREGION_SIGNATURE "IntelGFXMem"
>  #define MBOX_VBT      (1<<3)
>  
>  /* device handle */

-- 
Jani Nikula, Intel