[PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Dimitrije Pavlov]

Ensure that the PixelInformation field of the
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is zeroed out in
EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode() and
EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() when PixelFormat is
PixelBlueGreenRedReserved8BitPerColor.

According to UEFI 2.9 Section 12.9, PixelInformation field of the
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is valid only if
PixelFormat is PixelBitMask. This means that firmware is not required
to fill out the PixelInformation field for other PixelFormat types,
which implies that the QemuVideoDxe implementation is technically
correct.

However, not zeroing out those fields will leak the contents of the
memory returned by the memory allocator, so it is better to explicitly
set them to zero.

In addition, the SCT test suite relies on PixelInformation always
having a consistent value, which causes failures.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeremy Linton <Jeremy.Linton@arm.com>

Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
---
 OvmfPkg/QemuVideoDxe/Gop.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 0c4dea7fb6f2..7a9fe208c99c 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -31,7 +31,14 @@ QemuVideoCompleteModeInfo (
     Info->PixelInformation.ReservedMask = 0;
   } else if (ModeData->ColorDepth == 32) {
     DEBUG ((DEBUG_INFO, "PixelBlueGreenRedReserved8BitPerColor\n"));
-    Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
+    Info->PixelFormat                   = PixelBlueGreenRedReserved8BitPerColor;
+    Info->PixelInformation.RedMask      = 0;
+    Info->PixelInformation.GreenMask    = 0;
+    Info->PixelInformation.BlueMask     = 0;
+    Info->PixelInformation.ReservedMask = 0;
+  } else {
+    DEBUG ((DEBUG_ERROR, "%a: Invalid ColorDepth %u", __FUNCTION__, ModeData->ColorDepth));
+    ASSERT (FALSE);
   }
 
   Info->PixelsPerScanLine = Info->HorizontalResolution;
-- 
2.34.1

Re: [edk2-devel] [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Gerd Hoffmann]

On Tue, Jun 28, 2022 at 01:48:16PM -0500, Dimitrije Pavlov wrote:
> Ensure that the PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is zeroed out in
> EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode() and
> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() when PixelFormat is
> PixelBlueGreenRedReserved8BitPerColor.
> 
> According to UEFI 2.9 Section 12.9, PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is valid only if
> PixelFormat is PixelBitMask. This means that firmware is not required
> to fill out the PixelInformation field for other PixelFormat types,
> which implies that the QemuVideoDxe implementation is technically
> correct.
> 
> However, not zeroing out those fields will leak the contents of the
> memory returned by the memory allocator, so it is better to explicitly
> set them to zero.
> 
> In addition, the SCT test suite relies on PixelInformation always
> having a consistent value, which causes failures.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Sunny Wang <Sunny.Wang@arm.com>
> Cc: Jeremy Linton <Jeremy.Linton@arm.com>
> 
> Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Sunny Wang]

Looks good to me. Thanks for working on this, Dimitrije.
I had an offline discussion with Dimitrije. Either this patch or the patch for SCT (https://edk2.groups.io/g/devel/topic/92068027) can fix the inconsistent test failure issue mentioned in https://bugzilla.tianocore.org/show_bug.cgi?id=3601

Reviewed-by: Sunny Wang <sunny.wang@arm.com>

-----Original Message-----
From: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
Sent: 28 June 2022 19:48
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Jeremy Linton <Jeremy.Linton@arm.com>
Subject: [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

Ensure that the PixelInformation field of the
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is zeroed out in
EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode() and
EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() when PixelFormat is
PixelBlueGreenRedReserved8BitPerColor.

According to UEFI 2.9 Section 12.9, PixelInformation field of the
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is valid only if
PixelFormat is PixelBitMask. This means that firmware is not required
to fill out the PixelInformation field for other PixelFormat types,
which implies that the QemuVideoDxe implementation is technically
correct.

However, not zeroing out those fields will leak the contents of the
memory returned by the memory allocator, so it is better to explicitly
set them to zero.

In addition, the SCT test suite relies on PixelInformation always
having a consistent value, which causes failures.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeremy Linton <Jeremy.Linton@arm.com>

Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
---
 OvmfPkg/QemuVideoDxe/Gop.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 0c4dea7fb6f2..7a9fe208c99c 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -31,7 +31,14 @@ QemuVideoCompleteModeInfo (
     Info->PixelInformation.ReservedMask = 0;
   } else if (ModeData->ColorDepth == 32) {
     DEBUG ((DEBUG_INFO, "PixelBlueGreenRedReserved8BitPerColor\n"));
-    Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
+    Info->PixelFormat                   = PixelBlueGreenRedReserved8BitPerColor;
+    Info->PixelInformation.RedMask      = 0;
+    Info->PixelInformation.GreenMask    = 0;
+    Info->PixelInformation.BlueMask     = 0;
+    Info->PixelInformation.ReservedMask = 0;
+  } else {
+    DEBUG ((DEBUG_ERROR, "%a: Invalid ColorDepth %u", __FUNCTION__, ModeData->ColorDepth));
+    ASSERT (FALSE);
   }

   Info->PixelsPerScanLine = Info->HorizontalResolution;
--
2.34.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Samer El-Haj-Mahmoud]

Reviewed-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>


Any chance we can get this pushed?

Thanks,
--Samer


> -----Original Message-----
> From: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
> Sent: Tuesday, June 28, 2022 2:48 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Jiewen Yao
> <jiewen.yao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Jeff Booher-Kaeding <Jeff.Booher-
> Kaeding@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Jeremy
> Linton <Jeremy.Linton@arm.com>
> Subject: [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out
> PixelInformation in QueryMode
> 
> Ensure that the PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is zeroed out in
> EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode() and
> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() when PixelFormat is
> PixelBlueGreenRedReserved8BitPerColor.
> 
> According to UEFI 2.9 Section 12.9, PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is valid only if
> PixelFormat is PixelBitMask. This means that firmware is not required
> to fill out the PixelInformation field for other PixelFormat types,
> which implies that the QemuVideoDxe implementation is technically
> correct.
> 
> However, not zeroing out those fields will leak the contents of the
> memory returned by the memory allocator, so it is better to explicitly
> set them to zero.
> 
> In addition, the SCT test suite relies on PixelInformation always
> having a consistent value, which causes failures.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Sunny Wang <Sunny.Wang@arm.com>
> Cc: Jeremy Linton <Jeremy.Linton@arm.com>
> 
> Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
> ---
>  OvmfPkg/QemuVideoDxe/Gop.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c
> b/OvmfPkg/QemuVideoDxe/Gop.c
> index 0c4dea7fb6f2..7a9fe208c99c 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -31,7 +31,14 @@ QemuVideoCompleteModeInfo (
>      Info->PixelInformation.ReservedMask = 0;
>    } else if (ModeData->ColorDepth == 32) {
>      DEBUG ((DEBUG_INFO, "PixelBlueGreenRedReserved8BitPerColor\n"));
> -    Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
> +    Info->PixelFormat                   =
> PixelBlueGreenRedReserved8BitPerColor;
> +    Info->PixelInformation.RedMask      = 0;
> +    Info->PixelInformation.GreenMask    = 0;
> +    Info->PixelInformation.BlueMask     = 0;
> +    Info->PixelInformation.ReservedMask = 0;
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a: Invalid ColorDepth %u", __FUNCTION__,
> ModeData->ColorDepth));
> +    ASSERT (FALSE);
>    }
> 
>    Info->PixelsPerScanLine = Info->HorizontalResolution;
> --
> 2.34.1

Re: [edk2-devel] [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Ard Biesheuvel]

On Tue, 28 Jun 2022 at 11:48, Dimitrije Pavlov <Dimitrije.Pavlov@arm.com> wrote:
>
> Ensure that the PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is zeroed out in
> EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode() and
> EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode() when PixelFormat is
> PixelBlueGreenRedReserved8BitPerColor.
>
> According to UEFI 2.9 Section 12.9, PixelInformation field of the
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION structure is valid only if
> PixelFormat is PixelBitMask. This means that firmware is not required
> to fill out the PixelInformation field for other PixelFormat types,
> which implies that the QemuVideoDxe implementation is technically
> correct.
>
> However, not zeroing out those fields will leak the contents of the
> memory returned by the memory allocator, so it is better to explicitly
> set them to zero.
>
> In addition, the SCT test suite relies on PixelInformation always
> having a consistent value, which causes failures.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Sunny Wang <Sunny.Wang@arm.com>
> Cc: Jeremy Linton <Jeremy.Linton@arm.com>
>
> Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
> ---
>  OvmfPkg/QemuVideoDxe/Gop.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 0c4dea7fb6f2..7a9fe208c99c 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -31,7 +31,14 @@ QemuVideoCompleteModeInfo (
>      Info->PixelInformation.ReservedMask  0;
>    } else if (ModeData->ColorDepth  32) {
>      DEBUG ((DEBUG_INFO, "PixelBlueGreenRedReserved8BitPerColor\n"));
> -    Info->PixelFormat  PixelBlueGreenRedReserved8BitPerColor;
> +    Info->PixelFormat                    PixelBlueGreenRedReserved8BitPerColor;
> +    Info->PixelInformation.RedMask       0;
> +    Info->PixelInformation.GreenMask     0;
> +    Info->PixelInformation.BlueMask      0;
> +    Info->PixelInformation.ReservedMask  0;

Is this valid C? Or is the patch corrupted by email?


> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a: Invalid ColorDepth %u", __FUNCTION__, ModeData->ColorDepth));
> +    ASSERT (FALSE);
>    }
>
>    Info->PixelsPerScanLine  Info->HorizontalResolution;
> --
> 2.34.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#90822): https://edk2.groups.io/g/devel/message/90822
> Mute This Topic: https://groups.io/mt/92050521/1131722
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> ------------
>
>

Re: [edk2-devel] [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Dimitrije Pavlov]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

Looks like the patch is corrupted for you for some reason. Up the email chain it shows fine. You can see the patch here: https://edk2.groups.io/g/devel/message/90822

[-- Attachment #2: Type: text/html, Size: 262 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] OvmfPkg/QemuVideoDxe: Zero out PixelInformation in QueryMode

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]

Hi,

I also cannot see the patch properly. "=" disappearing magically sounds
like a quoted-printable email with a non-quoted-printable content.
Groups.io does not properly parse quoted-printable emails, so it's leaving
the "=" as is.

Thanks,
Pedro

On Wed, Jul 27, 2022 at 8:34 PM Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
wrote:

> Looks like the patch is corrupted for you for some reason. Up the email
> chain it shows fine. You can see the patch here:
> https://edk2.groups.io/g/devel/message/90822
> 
>
>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 1103 bytes --]