[PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator per UEFI spec

[PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator per UEFI spec

[Neal Gompa]

Per UEFI Spec 2.8 (UEFI_Spec_2_8_final.pdf, page 114)
5.2.3 Protective MBR
Table 20. Protective MBR Partition Record protecting the entire disk

The description for BootIndicator states the following:

> Set to 0x00 to indicate a non-bootable partition. If set to any
> value other than 0x00 the behavior of this flag on non-UEFI
> systems is undefined. Must be ignored by UEFI implementations.

Unfortunately, we have been incorrectly assuming that the
BootIndicator value must be 0x00, which leads to problems
when the 'pmbr_boot' flag is set on a disk containing a GPT
(such as with GNU parted). When the flag is set, the value
changes to 0x01, causing this check to fail and the system
is rendered unbootable despite it being valid from the
perspective of the UEFI spec.

To resolve this, we drop the check for the BootIndicator
so that we stop caring about the value set there, which
restores the capability to boot such disks.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3474

Cc: Chris Murphy <chrismurphy@fedoraproject.org>
Cc: David Duncan <davdunc@amazon.com>
Cc: Lazlo Ersek <lersek@redhat.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>

Signed-off-by: Neal Gompa <ngompa@fedoraproject.org>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
index aefb2d6ecb3f..efaff5e0808f 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
@@ -264,8 +264,7 @@ PartitionInstallGptChildHandles (
   // Verify that the Protective MBR is valid
   //
   for (Index = 0; Index < MAX_MBR_PARTITIONS; Index++) {
-    if (ProtectiveMbr->Partition[Index].BootIndicator == 0x00 &&
-        ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
+    if (ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
         UNPACK_UINT32 (ProtectiveMbr->Partition[Index].StartingLBA) == 1
         ) {
       break;
-- 
2.31.1

Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator per UEFI spec

[Laszlo Ersek]

On 07/05/21 11:36, Neal Gompa wrote:
> Per UEFI Spec 2.8 (UEFI_Spec_2_8_final.pdf, page 114)
> 5.2.3 Protective MBR
> Table 20. Protective MBR Partition Record protecting the entire disk
> 
> The description for BootIndicator states the following:
> 
>> Set to 0x00 to indicate a non-bootable partition. If set to any
>> value other than 0x00 the behavior of this flag on non-UEFI
>> systems is undefined. Must be ignored by UEFI implementations.
> 
> Unfortunately, we have been incorrectly assuming that the
> BootIndicator value must be 0x00, which leads to problems
> when the 'pmbr_boot' flag is set on a disk containing a GPT
> (such as with GNU parted). When the flag is set, the value
> changes to 0x01, causing this check to fail and the system
> is rendered unbootable despite it being valid from the
> perspective of the UEFI spec.
> 
> To resolve this, we drop the check for the BootIndicator
> so that we stop caring about the value set there, which
> restores the capability to boot such disks.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3474
> 
> Cc: Chris Murphy <chrismurphy@fedoraproject.org>
> Cc: David Duncan <davdunc@amazon.com>
> Cc: Lazlo Ersek <lersek@redhat.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> 
> Signed-off-by: Neal Gompa <ngompa@fedoraproject.org>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> index aefb2d6ecb3f..efaff5e0808f 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> @@ -264,8 +264,7 @@ PartitionInstallGptChildHandles (
>    // Verify that the Protective MBR is valid
>    //
>    for (Index = 0; Index < MAX_MBR_PARTITIONS; Index++) {
> -    if (ProtectiveMbr->Partition[Index].BootIndicator == 0x00 &&
> -        ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
> +    if (ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
>          UNPACK_UINT32 (ProtectiveMbr->Partition[Index].StartingLBA) == 1
>          ) {
>        break;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator per UEFI spec

[Wu, Hao A]

> -----Original Message-----
> From: Neal Gompa <ngompa@fedoraproject.org>
> Sent: Monday, July 5, 2021 5:36 PM
> To: devel@edk2.groups.io
> Cc: Neal Gompa <ngompa@fedoraproject.org>; Chris Murphy
> <chrismurphy@fedoraproject.org>; David Duncan <davdunc@amazon.com>;
> Lazlo Ersek <lersek@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: [PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator
> per UEFI spec
> 
> Per UEFI Spec 2.8 (UEFI_Spec_2_8_final.pdf, page 114)
> 5.2.3 Protective MBR
> Table 20. Protective MBR Partition Record protecting the entire disk
> 
> The description for BootIndicator states the following:
> 
> > Set to 0x00 to indicate a non-bootable partition. If set to any
> > value other than 0x00 the behavior of this flag on non-UEFI
> > systems is undefined. Must be ignored by UEFI implementations.
> 
> Unfortunately, we have been incorrectly assuming that the
> BootIndicator value must be 0x00, which leads to problems
> when the 'pmbr_boot' flag is set on a disk containing a GPT
> (such as with GNU parted). When the flag is set, the value
> changes to 0x01, causing this check to fail and the system
> is rendered unbootable despite it being valid from the
> perspective of the UEFI spec.
> 
> To resolve this, we drop the check for the BootIndicator
> so that we stop caring about the value set there, which
> restores the capability to boot such disks.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3474
> 
> Cc: Chris Murphy <chrismurphy@fedoraproject.org>
> Cc: David Duncan <davdunc@amazon.com>
> Cc: Lazlo Ersek <lersek@redhat.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> 
> Signed-off-by: Neal Gompa <ngompa@fedoraproject.org>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> index aefb2d6ecb3f..efaff5e0808f 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> @@ -264,8 +264,7 @@ PartitionInstallGptChildHandles (
>    // Verify that the Protective MBR is valid
> 
>    //
> 
>    for (Index = 0; Index < MAX_MBR_PARTITIONS; Index++) {
> 
> -    if (ProtectiveMbr->Partition[Index].BootIndicator == 0x00 &&
> 
> -        ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION
> &&
> 
> +    if (ProtectiveMbr->Partition[Index].OSIndicator ==
> PMBR_GPT_PARTITION &&
> 


Thanks a lot for the fix.
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>          UNPACK_UINT32 (ProtectiveMbr->Partition[Index].StartingLBA) == 1
> 
>          ) {
> 
>        break;
> 
> --
> 2.31.1

Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Ignore PMBR BootIndicator per UEFI spec

[Laszlo Ersek]

On 07/05/21 11:36, Neal Gompa wrote:
> Per UEFI Spec 2.8 (UEFI_Spec_2_8_final.pdf, page 114)
> 5.2.3 Protective MBR
> Table 20. Protective MBR Partition Record protecting the entire disk
> 
> The description for BootIndicator states the following:
> 
>> Set to 0x00 to indicate a non-bootable partition. If set to any
>> value other than 0x00 the behavior of this flag on non-UEFI
>> systems is undefined. Must be ignored by UEFI implementations.
> 
> Unfortunately, we have been incorrectly assuming that the
> BootIndicator value must be 0x00, which leads to problems
> when the 'pmbr_boot' flag is set on a disk containing a GPT
> (such as with GNU parted). When the flag is set, the value
> changes to 0x01, causing this check to fail and the system
> is rendered unbootable despite it being valid from the
> perspective of the UEFI spec.
> 
> To resolve this, we drop the check for the BootIndicator
> so that we stop caring about the value set there, which
> restores the capability to boot such disks.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3474
> 
> Cc: Chris Murphy <chrismurphy@fedoraproject.org>
> Cc: David Duncan <davdunc@amazon.com>
> Cc: Lazlo Ersek <lersek@redhat.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> 
> Signed-off-by: Neal Gompa <ngompa@fedoraproject.org>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> index aefb2d6ecb3f..efaff5e0808f 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Gpt.c
> @@ -264,8 +264,7 @@ PartitionInstallGptChildHandles (
>    // Verify that the Protective MBR is valid
>    //
>    for (Index = 0; Index < MAX_MBR_PARTITIONS; Index++) {
> -    if (ProtectiveMbr->Partition[Index].BootIndicator == 0x00 &&
> -        ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
> +    if (ProtectiveMbr->Partition[Index].OSIndicator == PMBR_GPT_PARTITION &&
>          UNPACK_UINT32 (ProtectiveMbr->Partition[Index].StartingLBA) == 1
>          ) {
>        break;
> 

Merged as commit b3db0cb1f8d1, via
<https://github.com/tianocore/edk2/pull/1792>.

I fixed up the authorship meta-datum with "git commit --amend
--author='Neal Gompa <ngompa@fedoraproject.org>'", per BZ comment
<https://bugzilla.tianocore.org/show_bug.cgi?id=3474#c9>.

Thanks
Laszlo