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

2022-08-01 Thread Ard Biesheuvel 
On Thu, 28 Jul 2022 at 23:31, 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 
> Cc: Jiewen Yao 
> Cc: Jordan Justen 
> Cc: Gerd Hoffmann 
> Cc: Jeff Booher-Kaeding 
> Cc: Samer El-Haj-Mahmoud 
> Cc: Sunny Wang 
> Cc: Jeremy Linton 
>
> Signed-off-by: Dimitrije Pavlov 
>
> Acked-by: Gerd Hoffmann 

Merged as #3164

Thanks,

> ---
>  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
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92031): https://edk2.groups.io/g/devel/message/92031
Mute This Topic: https://groups.io/mt/92679973/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2022-07-28 Thread 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 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Gerd Hoffmann 
Cc: Jeff Booher-Kaeding 
Cc: Samer El-Haj-Mahmoud 
Cc: Sunny Wang 
Cc: Jeremy Linton 

Signed-off-by: Dimitrije Pavlov 

Acked-by: Gerd Hoffmann 
---
 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91953): https://edk2.groups.io/g/devel/message/91953
Mute This Topic: https://groups.io/mt/92679973/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2022-07-28 Thread 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 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Gerd Hoffmann 
Cc: Jeff Booher-Kaeding 
Cc: Samer El-Haj-Mahmoud 
Cc: Sunny Wang 
Cc: Jeremy Linton 

Signed-off-by: Dimitrije Pavlov 

Acked-by: Gerd Hoffmann 
---
 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