Date
1 - 7 of 7
[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@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Jordan Justen <jordan.l.justen@...> Cc: Gerd Hoffmann <kraxel@...> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@...> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...> Cc: Sunny Wang <Sunny.Wang@...> Cc: Jeremy Linton <Jeremy.Linton@...> Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@...> --- 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 |
|
|
|
Gerd Hoffmann
On Tue, Jun 28, 2022 at 01:48:16PM -0500, Dimitrije Pavlov wrote:
Ensure that the PixelInformation field of theAcked-by: Gerd Hoffmann <kraxel@...> |
|
|
|
Sunny Wang
Looks good to me. Thanks for working on this, Dimitrije.
toggle quoted message
Show quoted text
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@...> -----Original Message-----
From: Dimitrije Pavlov <Dimitrije.Pavlov@...> Sent: 28 June 2022 19:48 To: devel@edk2.groups.io Cc: Ard Biesheuvel <ardb+tianocore@...>; Jiewen Yao <jiewen.yao@...>; Jordan Justen <jordan.l.justen@...>; Gerd Hoffmann <kraxel@...>; Jeff Booher-Kaeding <Jeff.Booher-Kaeding@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; Sunny Wang <Sunny.Wang@...>; Jeremy Linton <Jeremy.Linton@...> 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@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Jordan Justen <jordan.l.justen@...> Cc: Gerd Hoffmann <kraxel@...> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@...> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...> Cc: Sunny Wang <Sunny.Wang@...> Cc: Jeremy Linton <Jeremy.Linton@...> Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@...> --- 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. |
|
|
|
Samer El-Haj-Mahmoud
Reviewed-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
toggle quoted message
Show quoted text
Any chance we can get this pushed? Thanks, --Samer -----Original Message----- |
|
|
|
Ard Biesheuvel
On Tue, 28 Jun 2022 at 11:48, Dimitrije Pavlov <Dimitrije.Pavlov@...> wrote:
Is this valid C? Or is the patch corrupted by email? + } else { |
|
|
|
Dimitrije Pavlov
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
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@...> 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 |
|
|