Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Robin Holt]

If I revert that commit (5dcd14ecd4), It completes boot fine.  With that
commit applied, by ACPI tables (as well as much else) seems to be
completely messed up.  The ACPI XSDT table is removed, all the SRAT
is gone.  Much of the remaining ACPI table is messed up.  Some of the
EFI memory map is whacked.

If I change the following, things work:
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 5b5e9cb..85337ab 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -19,9 +19,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 {
        if (boot_params->sentinel) {
                /*fields in boot_params are not valid, clear them */
+#if 0
                memset(&boot_params->olpc_ofw_header, 0,
                       (char *)&boot_params->alt_mem_k -
                        (char *)&boot_params->olpc_ofw_header);
+#endif
                memset(&boot_params->kbd_status, 0,
                       (char *)&boot_params->hdr -
                       (char *)&boot_params->kbd_status);

Thanks,
Robin

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 02/28/2013 12:52 PM, Robin Holt wrote:
> If I revert that commit (5dcd14ecd4), It completes boot fine.  With that
> commit applied, by ACPI tables (as well as much else) seems to be
> completely messed up.  The ACPI XSDT table is removed, all the SRAT
> is gone.  Much of the remaining ACPI table is messed up.  Some of the
> EFI memory map is whacked.
> 
> If I change the following, things work:
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index 5b5e9cb..85337ab 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -19,9 +19,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
>  {
>         if (boot_params->sentinel) {
>                 /*fields in boot_params are not valid, clear them */
> +#if 0
>                 memset(&boot_params->olpc_ofw_header, 0,
>                        (char *)&boot_params->alt_mem_k -
>                         (char *)&boot_params->olpc_ofw_header);
> +#endif
>                 memset(&boot_params->kbd_status, 0,
>                        (char *)&boot_params->hdr -
>                        (char *)&boot_params->kbd_status);
> 

Sounds like ELILO needs to be fixed to pass the boot_params correctly.

To support existing ELILO, it would be good to know exactly *which*
fields ELILO pass in as garbage (because it does, that is why ->sentinel
is nonzero) and use its bootloader ID to apply the proper brainwhack for
the legacy versions.

	-hpa

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Robin Holt]

On Thu, Feb 28, 2013 at 01:05:27PM -0800, H. Peter Anvin wrote:
> On 02/28/2013 12:52 PM, Robin Holt wrote:
> > If I revert that commit (5dcd14ecd4), It completes boot fine.  With that
> > commit applied, by ACPI tables (as well as much else) seems to be
> > completely messed up.  The ACPI XSDT table is removed, all the SRAT
> > is gone.  Much of the remaining ACPI table is messed up.  Some of the
> > EFI memory map is whacked.
> > 
> > If I change the following, things work:
> > diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> > index 5b5e9cb..85337ab 100644
> > --- a/arch/x86/include/asm/bootparam_utils.h
> > +++ b/arch/x86/include/asm/bootparam_utils.h
> > @@ -19,9 +19,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
> >  {
> >         if (boot_params->sentinel) {
> >                 /*fields in boot_params are not valid, clear them */
> > +#if 0
> >                 memset(&boot_params->olpc_ofw_header, 0,
> >                        (char *)&boot_params->alt_mem_k -
> >                         (char *)&boot_params->olpc_ofw_header);
> > +#endif
> >                 memset(&boot_params->kbd_status, 0,
> >                        (char *)&boot_params->hdr -
> >                        (char *)&boot_params->kbd_status);
> > 
> 
> Sounds like ELILO needs to be fixed to pass the boot_params correctly.
> 
> To support existing ELILO, it would be good to know exactly *which*
> fields ELILO pass in as garbage (because it does, that is why ->sentinel
> is nonzero) and use its bootloader ID to apply the proper brainwhack for
> the legacy versions.

Any idea how I can dump that information?

Robin

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 02/28/2013 01:09 PM, Robin Holt wrote:
>>
>> Sounds like ELILO needs to be fixed to pass the boot_params correctly.
>>
>> To support existing ELILO, it would be good to know exactly *which*
>> fields ELILO pass in as garbage (because it does, that is why ->sentinel
>> is nonzero) and use its bootloader ID to apply the proper brainwhack for
>> the legacy versions.
> 
> Any idea how I can dump that information?
> 

Look at the source code and look at what fields it actually initializes.
 Give us that list, and then we can work around elilo braindamage in the
kernel.

Then make it follow the boot spec:

> In 32-bit boot protocol, the first step in loading a Linux kernel
> should be to setup the boot parameters (struct boot_params,
> traditionally known as "zero page"). The memory for struct boot_params
> should be allocated and initialized to all zero. Then the setup header
> from offset 0x01f1 of kernel image on should be loaded into struct
> boot_params and examined. The end of setup header can be calculated as
> follow:
> 
>         0x0202 + byte value at offset 0x0201

... so we don't have to.

	-hpa

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Yinghai Lu]

On Thu, Feb 28, 2013 at 1:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/28/2013 01:09 PM, Robin Holt wrote:
>>>
>>> Sounds like ELILO needs to be fixed to pass the boot_params correctly.
>>>
>>> To support existing ELILO, it would be good to know exactly *which*
>>> fields ELILO pass in as garbage (because it does, that is why ->sentinel
>>> is nonzero) and use its bootloader ID to apply the proper brainwhack for
>>> the legacy versions.
>>
>> Any idea how I can dump that information?
>>
>
> Look at the source code and look at what fields it actually initializes.
>  Give us that list, and then we can work around elilo braindamage in the
> kernel.
>
> Then make it follow the boot spec:
>
>> In 32-bit boot protocol, the first step in loading a Linux kernel
>> should be to setup the boot parameters (struct boot_params,
>> traditionally known as "zero page"). The memory for struct boot_params
>> should be allocated and initialized to all zero. Then the setup header
>> from offset 0x01f1 of kernel image on should be loaded into struct
>> boot_params and examined. The end of setup header can be calculated as
>> follow:
>>
>>         0x0202 + byte value at offset 0x0201
>
> ... so we don't have to.

Good, kexec is not only one.

ELILO has one.
        5  ELILO

We need switch/case to have different fields set for it.

Thanks

Yinghai

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 02/28/2013 03:02 PM, Yinghai Lu wrote:
> 
> Good, kexec is not only one.
> 
> ELILO has one.
>         5  ELILO
> 
> We need switch/case to have different fields set for it.
> 

I wouldn't consider that "good", but it's a bloody good thing we have
bootloader IDs.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Robin Holt]

On Thu, Feb 28, 2013 at 03:09:34PM -0800, H. Peter Anvin wrote:
> On 02/28/2013 03:02 PM, Yinghai Lu wrote:
> > 
> > Good, kexec is not only one.
> > 
> > ELILO has one.
> >         5  ELILO
> > 
> > We need switch/case to have different fields set for it.
> > 
> 
> I wouldn't consider that "good", but it's a bloody good thing we have
> bootloader IDs.

What is the planned course of action here.  With this commit applied,
I still cannot boot.  Is there something you are waiting for from me?

Thanks,
Robin

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

Yes, please do the analysis I asked for.

Robin Holt <holt@sgi.com> wrote:

>On Thu, Feb 28, 2013 at 03:09:34PM -0800, H. Peter Anvin wrote:
>> On 02/28/2013 03:02 PM, Yinghai Lu wrote:
>> > 
>> > Good, kexec is not only one.
>> > 
>> > ELILO has one.
>> >         5  ELILO
>> > 
>> > We need switch/case to have different fields set for it.
>> > 
>> 
>> I wouldn't consider that "good", but it's a bloody good thing we have
>> bootloader IDs.
>
>What is the planned course of action here.  With this commit applied,
>I still cannot boot.  Is there something you are waiting for from me?
>
>Thanks,
>Robin

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Yinghai Lu]

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

On Tue, Mar 5, 2013 at 7:22 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> Yes, please do the analysis I asked for.

it uses first 2 pages in bzImage to bootparams.

then update the fields with ===> X

struct boot_params {
        struct screen_info screen_info;                 /* 0x000 */   ===> X
        struct apm_bios_info apm_bios_info;             /* 0x040 */   ===> X
        __u8  _pad2[4];                                 /* 0x054 */
        __u64  tboot_addr;                              /* 0x058 */
        struct ist_info ist_info;                       /* 0x060 */
        __u8  _pad3[16];                                /* 0x070 */
        __u8  hd0_info[16];     /* obsolete! */         /* 0x080 */   ===> X
        __u8  hd1_info[16];     /* obsolete! */         /* 0x090 */   ===> X
        struct sys_desc_table sys_desc_table;           /* 0x0a0 */   ===> X
        struct olpc_ofw_header olpc_ofw_header;         /* 0x0b0 */
        __u32 ext_ramdisk_image;                        /* 0x0c0 */
        __u32 ext_ramdisk_size;                         /* 0x0c4 */
        __u32 ext_cmd_line_ptr;                         /* 0x0c8 */
        __u8  _pad4[116];                               /* 0x0cc */
        struct edid_info edid_info;                     /* 0x140 */
        struct efi_info efi_info;                       /* 0x1c0 */   ===> X
        __u32 alt_mem_k;                                /* 0x1e0 */   ===> X
        __u32 scratch;          /* Scratch field! */    /* 0x1e4 */
        __u8  e820_entries;                             /* 0x1e8 */  ===> X
        __u8  eddbuf_entries;                           /* 0x1e9 */
        __u8  edd_mbr_sig_buf_entries;                  /* 0x1ea */
        __u8  kbd_status;                               /* 0x1eb */
        __u8  _pad5[3];                                 /* 0x1ec */
        /*
         * The sentinel is set to a nonzero value (0xff) in header.S.
         *
         * A bootloader is supposed to only take setup_header and put
         * it into a clean boot_params buffer. If it turns out that
         * it is clumsy or too generous with the buffer, it most
         * probably will pick up the sentinel variable too. The fact
         * that this variable then is still 0xff will let kernel
         * know that some variables in boot_params are invalid and
         * kernel should zero out certain portions of boot_params.
         */
        __u8  sentinel;                                 /* 0x1ef */
        __u8  _pad6[1];                                 /* 0x1f0 */
        struct setup_header hdr;    /* setup header */  /* 0x1f1 */  ===> X
        __u8  _pad7[0x290-0x1f1-sizeof(struct setup_header)];
        __u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX];      /* 0x290 */
        struct e820entry e820_map[E820MAX];             /* 0x2d0 */  ===> X
        __u8  _pad8[48];                                /* 0xcd0 */
        struct edd_info eddbuf[EDDMAXNR];               /* 0xd00 */
        __u8  _pad9[276];                               /* 0xeec */

so sentinel will be kept as 0xff, so efi_info get cleared by kernel...

Attached patches should avoid the clearing of efi_info when elilo is used.

Do we need to clear edd and pad2 and pad3 for elilo?

Thanks

Yinghai

[-- Attachment #2: fix_elilo.patch --]
[-- Type: application/octet-stream, Size: 925 bytes --]

---
 arch/x86/include/asm/bootparam_utils.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/include/asm/bootparam_utils.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/bootparam_utils.h
+++ linux-2.6/arch/x86/include/asm/bootparam_utils.h
@@ -20,8 +20,11 @@ static void sanitize_boot_params(struct
 	if (boot_params->sentinel) {
 		/*fields in boot_params are not valid, clear them */
 		memset(&boot_params->olpc_ofw_header, 0,
-		       (char *)&boot_params->alt_mem_k -
+		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->olpc_ofw_header);
+		if (boot_params->hdr.type_of_loader != 0x50) /* not eflio */
+			memset(&boot_params->efi_info, 0,
+					 sizeof(boot_params->efi_info));
 		memset(&boot_params->kbd_status, 0,
 		       (char *)&boot_params->hdr -
 		       (char *)&boot_params->kbd_status);

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Robin Holt]

That fixed it for me.

Can you help me understand why sentinel is non-zero?  It looks to me
like 3.14 allocates 16kB plus strlen of the command line, zeros it,
and then proceeds to fill in fields, some differing from what is in the
boot_params structure.  That said, it looks like the sentinel field
should remain 0.  I am still trying to understand, but if this patch
makes it in, I am happy.

Thanks,
Robin

On Tue, Mar 05, 2013 at 11:12:08AM -0800, Yinghai Lu wrote:
> On Tue, Mar 5, 2013 at 7:22 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> > Yes, please do the analysis I asked for.
> 
> it uses first 2 pages in bzImage to bootparams.
> 
> then update the fields with ===> X
> 
> struct boot_params {
>         struct screen_info screen_info;                 /* 0x000 */   ===> X
>         struct apm_bios_info apm_bios_info;             /* 0x040 */   ===> X
>         __u8  _pad2[4];                                 /* 0x054 */
>         __u64  tboot_addr;                              /* 0x058 */
>         struct ist_info ist_info;                       /* 0x060 */
>         __u8  _pad3[16];                                /* 0x070 */
>         __u8  hd0_info[16];     /* obsolete! */         /* 0x080 */   ===> X
>         __u8  hd1_info[16];     /* obsolete! */         /* 0x090 */   ===> X
>         struct sys_desc_table sys_desc_table;           /* 0x0a0 */   ===> X
>         struct olpc_ofw_header olpc_ofw_header;         /* 0x0b0 */
>         __u32 ext_ramdisk_image;                        /* 0x0c0 */
>         __u32 ext_ramdisk_size;                         /* 0x0c4 */
>         __u32 ext_cmd_line_ptr;                         /* 0x0c8 */
>         __u8  _pad4[116];                               /* 0x0cc */
>         struct edid_info edid_info;                     /* 0x140 */
>         struct efi_info efi_info;                       /* 0x1c0 */   ===> X
>         __u32 alt_mem_k;                                /* 0x1e0 */   ===> X
>         __u32 scratch;          /* Scratch field! */    /* 0x1e4 */
>         __u8  e820_entries;                             /* 0x1e8 */  ===> X
>         __u8  eddbuf_entries;                           /* 0x1e9 */
>         __u8  edd_mbr_sig_buf_entries;                  /* 0x1ea */
>         __u8  kbd_status;                               /* 0x1eb */
>         __u8  _pad5[3];                                 /* 0x1ec */
>         /*
>          * The sentinel is set to a nonzero value (0xff) in header.S.
>          *
>          * A bootloader is supposed to only take setup_header and put
>          * it into a clean boot_params buffer. If it turns out that
>          * it is clumsy or too generous with the buffer, it most
>          * probably will pick up the sentinel variable too. The fact
>          * that this variable then is still 0xff will let kernel
>          * know that some variables in boot_params are invalid and
>          * kernel should zero out certain portions of boot_params.
>          */
>         __u8  sentinel;                                 /* 0x1ef */
>         __u8  _pad6[1];                                 /* 0x1f0 */
>         struct setup_header hdr;    /* setup header */  /* 0x1f1 */  ===> X
>         __u8  _pad7[0x290-0x1f1-sizeof(struct setup_header)];
>         __u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX];      /* 0x290 */
>         struct e820entry e820_map[E820MAX];             /* 0x2d0 */  ===> X
>         __u8  _pad8[48];                                /* 0xcd0 */
>         struct edd_info eddbuf[EDDMAXNR];               /* 0xd00 */
>         __u8  _pad9[276];                               /* 0xeec */
> 
> so sentinel will be kept as 0xff, so efi_info get cleared by kernel...
> 
> Attached patches should avoid the clearing of efi_info when elilo is used.
> 
> Do we need to clear edd and pad2 and pad3 for elilo?
> 
> Thanks
> 
> Yinghai

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Yinghai Lu]

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

On Tue, Mar 5, 2013 at 11:52 AM, Robin Holt <holt@sgi.com> wrote:
> That fixed it for me.
>
> Can you help me understand why sentinel is non-zero?  It looks to me
> like 3.14 allocates 16kB plus strlen of the command line, zeros it,
> and then proceeds to fill in fields, some differing from what is in the
> boot_params structure.  That said, it looks like the sentinel field
> should remain 0.  I am still trying to understand, but if this patch
> makes it in, I am happy.

sentinel is out of setup_header.
it is 0x1ef. setup_header is from 0x1f1.

it will be 0xff from arch/x86/boot/header.S for bzImage.

        .section ".header", "a"
        .globl  sentinel
sentinel:       .byte 0xff, 0xff        /* Used to detect broken loaders */

elilo first copy first page, and get 0x1f1 for setup code sector number.
then it copies all setup code ( include setup header).

after that sysdeps_create_boot_params will copy first two pages to bp...
CopyMem(bp, param_start, 0x2000);

in that case, it will keep all fields in boot_params around setup_header as
with setup code...thus 0x1ef will be 0xff.

elilo need attached fix.

Thanks

Yinghai

[-- Attachment #2: fix_boot_params.patch --]
[-- Type: application/octet-stream, Size: 1412 bytes --]

---
 ia32/system.c   |    9 ++++++++-
 x86_64/system.c |    9 ++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

Index: elilo/ia32/system.c
===================================================================
--- elilo.orig/ia32/system.c
+++ elilo/ia32/system.c
@@ -487,7 +487,14 @@ sysdeps_create_boot_params(
 	 * can be used by the command line.
 	 */
 	if (param_start != NULL) {
-		CopyMem(bp, param_start, 0x2000);
+		unsigned char *p = param_start;
+		unsigned long setup_header_size = p[0x201] + 0x202 - 0x1f1;
+
+		/* only copy setup_header */
+		if (setup_header_size > 0x7f)
+			setup_header_size = 0x7f;
+		CopyMem((unsigned char *)bp + 0x1f1, p + 0x1f1,
+			setup_header_size);
 		free(param_start);
 		param_start = NULL;
 		param_size = 0;
Index: elilo/x86_64/system.c
===================================================================
--- elilo.orig/x86_64/system.c
+++ elilo/x86_64/system.c
@@ -640,7 +640,14 @@ sysdeps_create_boot_params(
 	 * can be used by the command line.
 	 */
 	if (param_start != NULL) {
-		CopyMem(bp, param_start, 0x2000);
+		unsigned char *p = param_start;
+		unsigned long setup_header_size = p[0x201] + 0x202 - 0x1f1;
+
+		/* only copy setup_header */
+		if (setup_header_size > 0x7f)
+			setup_header_size = 0x7f;
+		CopyMem((unsigned char *)bp + 0x1f1, p + 0x1f1,
+			setup_header_size);
 		free(param_start);
 		param_start = NULL;
 		param_size = 0;

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Robin Holt]

On Tue, Mar 05, 2013 at 12:14:56PM -0800, Yinghai Lu wrote:
> On Tue, Mar 5, 2013 at 11:52 AM, Robin Holt <holt@sgi.com> wrote:
> > That fixed it for me.
> >
> > Can you help me understand why sentinel is non-zero?  It looks to me
> > like 3.14 allocates 16kB plus strlen of the command line, zeros it,
> > and then proceeds to fill in fields, some differing from what is in the
> > boot_params structure.  That said, it looks like the sentinel field
> > should remain 0.  I am still trying to understand, but if this patch
> > makes it in, I am happy.
> 
> sentinel is out of setup_header.
> it is 0x1ef. setup_header is from 0x1f1.
> 
> it will be 0xff from arch/x86/boot/header.S for bzImage.
> 
>         .section ".header", "a"
>         .globl  sentinel
> sentinel:       .byte 0xff, 0xff        /* Used to detect broken loaders */
> 
> elilo first copy first page, and get 0x1f1 for setup code sector number.
> then it copies all setup code ( include setup header).
> 
> after that sysdeps_create_boot_params will copy first two pages to bp...
> CopyMem(bp, param_start, 0x2000);

Had not gotten that far in my understanding of elilo.  Now I understand.

Robin

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Josh Boyer]

On Tue, Mar 5, 2013 at 2:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Mar 5, 2013 at 7:22 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>> Yes, please do the analysis I asked for.
>
> it uses first 2 pages in bzImage to bootparams.
>
> then update the fields with ===> X
>
> struct boot_params {
>         struct screen_info screen_info;                 /* 0x000 */   ===> X
>         struct apm_bios_info apm_bios_info;             /* 0x040 */   ===> X
>         __u8  _pad2[4];                                 /* 0x054 */
>         __u64  tboot_addr;                              /* 0x058 */
>         struct ist_info ist_info;                       /* 0x060 */
>         __u8  _pad3[16];                                /* 0x070 */
>         __u8  hd0_info[16];     /* obsolete! */         /* 0x080 */   ===> X
>         __u8  hd1_info[16];     /* obsolete! */         /* 0x090 */   ===> X
>         struct sys_desc_table sys_desc_table;           /* 0x0a0 */   ===> X
>         struct olpc_ofw_header olpc_ofw_header;         /* 0x0b0 */
>         __u32 ext_ramdisk_image;                        /* 0x0c0 */
>         __u32 ext_ramdisk_size;                         /* 0x0c4 */
>         __u32 ext_cmd_line_ptr;                         /* 0x0c8 */
>         __u8  _pad4[116];                               /* 0x0cc */
>         struct edid_info edid_info;                     /* 0x140 */
>         struct efi_info efi_info;                       /* 0x1c0 */   ===> X
>         __u32 alt_mem_k;                                /* 0x1e0 */   ===> X
>         __u32 scratch;          /* Scratch field! */    /* 0x1e4 */
>         __u8  e820_entries;                             /* 0x1e8 */  ===> X
>         __u8  eddbuf_entries;                           /* 0x1e9 */
>         __u8  edd_mbr_sig_buf_entries;                  /* 0x1ea */
>         __u8  kbd_status;                               /* 0x1eb */
>         __u8  _pad5[3];                                 /* 0x1ec */
>         /*
>          * The sentinel is set to a nonzero value (0xff) in header.S.
>          *
>          * A bootloader is supposed to only take setup_header and put
>          * it into a clean boot_params buffer. If it turns out that
>          * it is clumsy or too generous with the buffer, it most
>          * probably will pick up the sentinel variable too. The fact
>          * that this variable then is still 0xff will let kernel
>          * know that some variables in boot_params are invalid and
>          * kernel should zero out certain portions of boot_params.
>          */
>         __u8  sentinel;                                 /* 0x1ef */
>         __u8  _pad6[1];                                 /* 0x1f0 */
>         struct setup_header hdr;    /* setup header */  /* 0x1f1 */  ===> X
>         __u8  _pad7[0x290-0x1f1-sizeof(struct setup_header)];
>         __u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX];      /* 0x290 */
>         struct e820entry e820_map[E820MAX];             /* 0x2d0 */  ===> X
>         __u8  _pad8[48];                                /* 0xcd0 */
>         struct edd_info eddbuf[EDDMAXNR];               /* 0xd00 */
>         __u8  _pad9[276];                               /* 0xeec */
>
> so sentinel will be kept as 0xff, so efi_info get cleared by kernel...
>
> Attached patches should avoid the clearing of efi_info when elilo is used.
>
> Do we need to clear edd and pad2 and pad3 for elilo?

I don't think this is limited to elilo.  I have a UEFI machine booting
with grub2 that also fails to boot because of this patch.  I was in the
middle of bisecting when I found this thread and if I revert 5dcd14ecd4
the machine boots again.  Put that commit back in and it doesn't.  We've
had three other reports in Fedora of similar cases.

I discussed this with Peter Jones this morning.  He was looking into what
grub2 does for boot_params and it seems to be read-modify-write instead
of clearing the whole thing.  (CC'd Peter now.)

The patch for elilo probably works, but if grub2 is hitting this then I'm
curious if most bootloaders will.  I'll finish my bisect just to be extra
sure, but something probably needs to be done in a more generic fashion
here.

josh

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 03/06/2013 08:53 AM, Josh Boyer wrote:
> 
> I don't think this is limited to elilo.  I have a UEFI machine booting
> with grub2 that also fails to boot because of this patch.  I was in the
> middle of bisecting when I found this thread and if I revert 5dcd14ecd4
> the machine boots again.  Put that commit back in and it doesn't.  We've
> had three other reports in Fedora of similar cases.
> 
> I discussed this with Peter Jones this morning.  He was looking into what
> grub2 does for boot_params and it seems to be read-modify-write instead
> of clearing the whole thing.  (CC'd Peter now.)
> 
> The patch for elilo probably works, but if grub2 is hitting this then I'm
> curious if most bootloaders will.  I'll finish my bisect just to be extra
> sure, but something probably needs to be done in a more generic fashion
> here.
> 

Come to think about it...

The EFI field actually has a magic, unless just about all the rest of
them, so clearing it is probabilistically redundant; in other words we
almost certainly should exclude it from the clearing, unconditionally.

Does the "elilo" patch made unconditional work for you?

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Josh Boyer]

On Wed, Mar 6, 2013 at 12:26 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/06/2013 08:53 AM, Josh Boyer wrote:
>>
>> I don't think this is limited to elilo.  I have a UEFI machine booting
>> with grub2 that also fails to boot because of this patch.  I was in the
>> middle of bisecting when I found this thread and if I revert 5dcd14ecd4
>> the machine boots again.  Put that commit back in and it doesn't.  We've
>> had three other reports in Fedora of similar cases.
>>
>> I discussed this with Peter Jones this morning.  He was looking into what
>> grub2 does for boot_params and it seems to be read-modify-write instead
>> of clearing the whole thing.  (CC'd Peter now.)
>>
>> The patch for elilo probably works, but if grub2 is hitting this then I'm
>> curious if most bootloaders will.  I'll finish my bisect just to be extra
>> sure, but something probably needs to be done in a more generic fashion
>> here.
>>
>
> Come to think about it...
>
> The EFI field actually has a magic, unless just about all the rest of
> them, so clearing it is probabilistically redundant; in other words we
> almost certainly should exclude it from the clearing, unconditionally.
>
> Does the "elilo" patch made unconditional work for you?

Something like this?

Index: linux-2.6/arch/x86/include/asm/bootparam_utils.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/bootparam_utils.h
+++ linux-2.6/arch/x86/include/asm/bootparam_utils.h
@@ -20,8 +20,11 @@ static void sanitize_boot_params(struct
 	if (boot_params->sentinel) {
 		/*fields in boot_params are not valid, clear them */
 		memset(&boot_params->olpc_ofw_header, 0,
-		       (char *)&boot_params->alt_mem_k -
+		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->olpc_ofw_header);
 		memset(&boot_params->kbd_status, 0,
 		       (char *)&boot_params->hdr -
 		       (char *)&boot_params->kbd_status);

I can try that in a bit.

josh

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 03/06/2013 09:36 AM, Josh Boyer wrote:
> 
> Something like this?
> 
> Index: linux-2.6/arch/x86/include/asm/bootparam_utils.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/bootparam_utils.h
> +++ linux-2.6/arch/x86/include/asm/bootparam_utils.h
> @@ -20,8 +20,11 @@ static void sanitize_boot_params(struct
>  	if (boot_params->sentinel) {
>  		/*fields in boot_params are not valid, clear them */
>  		memset(&boot_params->olpc_ofw_header, 0,
> -		       (char *)&boot_params->alt_mem_k -
> +		       (char *)&boot_params->efi_info -
>  			(char *)&boot_params->olpc_ofw_header);
>  		memset(&boot_params->kbd_status, 0,
>  		       (char *)&boot_params->hdr -
>  		       (char *)&boot_params->kbd_status);
> 
> I can try that in a bit.
> 

Right.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Josh Boyer]

On Wed, Mar 6, 2013 at 12:37 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/06/2013 09:36 AM, Josh Boyer wrote:
>>
>> Something like this?
>>
>> Index: linux-2.6/arch/x86/include/asm/bootparam_utils.h
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/include/asm/bootparam_utils.h
>> +++ linux-2.6/arch/x86/include/asm/bootparam_utils.h
>> @@ -20,8 +20,11 @@ static void sanitize_boot_params(struct
>>       if (boot_params->sentinel) {
>>               /*fields in boot_params are not valid, clear them */
>>               memset(&boot_params->olpc_ofw_header, 0,
>> -                    (char *)&boot_params->alt_mem_k -
>> +                    (char *)&boot_params->efi_info -
>>                       (char *)&boot_params->olpc_ofw_header);
>>               memset(&boot_params->kbd_status, 0,
>>                      (char *)&boot_params->hdr -
>>                      (char *)&boot_params->kbd_status);
>>
>> I can try that in a bit.
>>
>
> Right.

OK.  Doing that on top of 3.9-rc1 results in me having a booting machine
again.  I'd suggest we get that into upstream quick-ish.

josh

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

Yeah.  I'll queue it up today and send to Linus in the next batch.

Josh Boyer <jwboyer@gmail.com> wrote:

>On Wed, Mar 6, 2013 at 12:37 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 03/06/2013 09:36 AM, Josh Boyer wrote:
>>>
>>> Something like this?
>>>
>>> Index: linux-2.6/arch/x86/include/asm/bootparam_utils.h
>>> ===================================================================
>>> --- linux-2.6.orig/arch/x86/include/asm/bootparam_utils.h
>>> +++ linux-2.6/arch/x86/include/asm/bootparam_utils.h
>>> @@ -20,8 +20,11 @@ static void sanitize_boot_params(struct
>>>       if (boot_params->sentinel) {
>>>               /*fields in boot_params are not valid, clear them */
>>>               memset(&boot_params->olpc_ofw_header, 0,
>>> -                    (char *)&boot_params->alt_mem_k -
>>> +                    (char *)&boot_params->efi_info -
>>>                       (char *)&boot_params->olpc_ofw_header);
>>>               memset(&boot_params->kbd_status, 0,
>>>                      (char *)&boot_params->hdr -
>>>                      (char *)&boot_params->kbd_status);
>>>
>>> I can try that in a bit.
>>>
>>
>> Right.
>
>OK.  Doing that on top of 3.9-rc1 results in me having a booting
>machine
>again.  I'd suggest we get that into upstream quick-ish.
>
>josh

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

[tip:x86/urgent] x86: Don' t clear efi_info even if the sentinel hits

[tip-bot for Josh Boyer]

Commit-ID:  2e604c0f19dcdd433b3863ffc3da9bc0787ca765
Gitweb:     http://git.kernel.org/tip/2e604c0f19dcdd433b3863ffc3da9bc0787ca765
Author:     Josh Boyer <jwboyer@gmail.com>
AuthorDate: Wed, 6 Mar 2013 20:23:30 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Wed, 6 Mar 2013 20:23:30 -0800

x86: Don't clear efi_info even if the sentinel hits

When boot_params->sentinel is set, all we really know is that some
undefined set of fields in struct boot_params contain garbage.  In the
particular case of efi_info, however, there is a private magic for
that substructure, so it is generally safe to leave it even if the
bootloader is broken.

kexec (for which we did the initial analysis) did not initialize this
field, but of course all the EFI bootloaders do, and most EFI
bootloaders are broken in this respect (and should be fixed.)

Reported-by: Robin Holt <holt@sgi.com>
Link: http://lkml.kernel.org/r/CA%2B5PVA51-FT14p4CRYKbicykugVb=PiaEycdQ57CK2km_OQuRQ@mail.gmail.com
Tested-by: Josh Boyer <jwboyer@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/bootparam_utils.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 5b5e9cb..ff808ef 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -14,13 +14,15 @@
  * analysis of kexec-tools; if other broken bootloaders initialize a
  * different set of fields we will need to figure out how to disambiguate.
  *
+ * Note: efi_info is commonly left uninitialized, but that field has a
+ * private magic, so it is better to leave it unchanged.
  */
 static void sanitize_boot_params(struct boot_params *boot_params)
 {
 	if (boot_params->sentinel) {
 		/*fields in boot_params are not valid, clear them */
 		memset(&boot_params->olpc_ofw_header, 0,
-		       (char *)&boot_params->alt_mem_k -
+		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->olpc_ofw_header);
 		memset(&boot_params->kbd_status, 0,
 		       (char *)&boot_params->hdr -

[PATCH] Be explicit about what the x86 0x020c boot parameter version requires.

[Peter Jones]

This should help avoid making the incorrect change in non-compliant
bootloaders.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 Documentation/x86/boot.txt             | 5 +++--
 arch/x86/include/asm/bootparam_utils.h | 7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
index 3840b6f..72702db 100644
--- a/Documentation/x86/boot.txt
+++ b/Documentation/x86/boot.txt
@@ -1110,7 +1110,8 @@ firmware, 'table' is the EFI system table - these are the first two
 arguments of the "handoff state" as described in section 2.3 of the
 UEFI specification. 'bp' is the boot loader-allocated boot params.
 
-The boot loader *must* fill out the following fields in bp,
+The boot loader *must* zero the entirity of bp, and then fill out the
+following fields:
 
     o hdr.code32_start
     o hdr.cmd_line_ptr
@@ -1118,4 +1119,4 @@ The boot loader *must* fill out the following fields in bp,
     o hdr.ramdisk_image (if applicable)
     o hdr.ramdisk_size  (if applicable)
 
-All other fields should be zero.
+All other fields should remain zero.
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 5b5e9cb..b4e8aa8 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -17,6 +17,13 @@
  */
 static void sanitize_boot_params(struct boot_params *boot_params)
 {
+	/* Note: do not simply clear this field.  The purpose of this field is
+	 * to guarantee compliance with the x86 boot spec located in
+	 * Documentation/x86/boot.txt .  That spec says that the *whole*
+	 * structure should be cleared.  If you're having an issue because
+	 * the sentinel is set, you need to change the whole structure to be
+	 * cleared, not this (or any other) indidivual field.
+	 */
 	if (boot_params->sentinel) {
 		/*fields in boot_params are not valid, clear them */
 		memset(&boot_params->olpc_ofw_header, 0,
-- 
1.8.1.4

Re: [PATCH] Be explicit about what the x86 0x020c boot parameter version requires.

[H. Peter Anvin]

On 03/06/2013 10:00 AM, Peter Jones wrote:
> This should help avoid making the incorrect change in non-compliant
> bootloaders.
> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  Documentation/x86/boot.txt             | 5 +++--
>  arch/x86/include/asm/bootparam_utils.h | 7 +++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
> index 3840b6f..72702db 100644
> --- a/Documentation/x86/boot.txt
> +++ b/Documentation/x86/boot.txt
> @@ -1110,7 +1110,8 @@ firmware, 'table' is the EFI system table - these are the first two
>  arguments of the "handoff state" as described in section 2.3 of the
>  UEFI specification. 'bp' is the boot loader-allocated boot params.
>  
> -The boot loader *must* fill out the following fields in bp,
> +The boot loader *must* zero the entirity of bp, and then fill out the
> +following fields:
>  
>      o hdr.code32_start
>      o hdr.cmd_line_ptr
> @@ -1118,4 +1119,4 @@ The boot loader *must* fill out the following fields in bp,
>      o hdr.ramdisk_image (if applicable)
>      o hdr.ramdisk_size  (if applicable)
>  

Wait a bloody minute here... I seem to have managed to miss something big.

Matt, should we not be copying the setup part of the structure just as
we do for the normal 32/64-bit protocol?  If not, why not?


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] Be explicit about what the x86 0x020c boot parameter version requires.

[Matt Fleming]

On Wed, 2013-03-06 at 20:31 -0800, H. Peter Anvin wrote:
> On 03/06/2013 10:00 AM, Peter Jones wrote:
> > This should help avoid making the incorrect change in non-compliant
> > bootloaders.
> > 
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> > ---
> >  Documentation/x86/boot.txt             | 5 +++--
> >  arch/x86/include/asm/bootparam_utils.h | 7 +++++++
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
> > index 3840b6f..72702db 100644
> > --- a/Documentation/x86/boot.txt
> > +++ b/Documentation/x86/boot.txt
> > @@ -1110,7 +1110,8 @@ firmware, 'table' is the EFI system table - these are the first two
> >  arguments of the "handoff state" as described in section 2.3 of the
> >  UEFI specification. 'bp' is the boot loader-allocated boot params.
> >  
> > -The boot loader *must* fill out the following fields in bp,
> > +The boot loader *must* zero the entirity of bp, and then fill out the
> > +following fields:
> >  
> >      o hdr.code32_start
> >      o hdr.cmd_line_ptr
> > @@ -1118,4 +1119,4 @@ The boot loader *must* fill out the following fields in bp,
> >      o hdr.ramdisk_image (if applicable)
> >      o hdr.ramdisk_size  (if applicable)
> >  
> 
> Wait a bloody minute here... I seem to have managed to miss something big.
> 
> Matt, should we not be copying the setup part of the structure just as
> we do for the normal 32/64-bit protocol?  If not, why not?

The structure that contains the hdr.version? Yeah, we should be copying
that.

[tip:x86/urgent] x86, doc: Be explicit about what the x86 struct boot_params requires

[tip-bot for Peter Jones]

Commit-ID:  3c4aff6b9a183b4f24eb7b8dd6c8a92cdba3bc75
Gitweb:     http://git.kernel.org/tip/3c4aff6b9a183b4f24eb7b8dd6c8a92cdba3bc75
Author:     Peter Jones <pjones@redhat.com>
AuthorDate: Wed, 6 Mar 2013 13:00:23 -0500
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Wed, 6 Mar 2013 20:34:43 -0800

x86, doc: Be explicit about what the x86 struct boot_params requires

If the sentinel triggers, we do not want the boot loader authors to
just poke it and make the error go away, we want them to actually fix
the problem.

This should help avoid making the incorrect change in non-compliant
bootloaders.

[ hpa: dropped the Documentation/x86/boot.txt hunk pending
  clarifications ]

Signed-off-by: Peter Jones <pjones@redhat.com>
Link: http://lkml.kernel.org/r/1362592823-28967-1-git-send-email-pjones@redhat.com
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/bootparam_utils.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index ff808ef..653668d 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -19,8 +19,22 @@
  */
 static void sanitize_boot_params(struct boot_params *boot_params)
 {
+	/* 
+	 * IMPORTANT NOTE TO BOOTLOADER AUTHORS: do not simply clear
+	 * this field.  The purpose of this field is to guarantee
+	 * compliance with the x86 boot spec located in
+	 * Documentation/x86/boot.txt .  That spec says that the
+	 * *whole* structure should be cleared, after which only the
+	 * portion defined by struct setup_header (boot_params->hdr)
+	 * should be copied in.
+	 *
+	 * If you're having an issue because the sentinel is set, you
+	 * need to change the whole structure to be cleared, not this
+	 * (or any other) individual field, or you will soon have
+	 * problems again.
+	 */
 	if (boot_params->sentinel) {
-		/*fields in boot_params are not valid, clear them */
+		/* fields in boot_params are left uninitialized, clear them */
 		memset(&boot_params->olpc_ofw_header, 0,
 		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->olpc_ofw_header);

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Peter Jones]

On Thu, Feb 28, 2013 at 01:12:11PM -0800, H. Peter Anvin wrote:
> Then make it follow the boot spec:
> 
> > In 32-bit boot protocol, the first step in loading a Linux kernel
> > should be to setup the boot parameters (struct boot_params,
> > traditionally known as "zero page"). The memory for struct boot_params
> > should be allocated and initialized to all zero. Then the setup header
> > from offset 0x01f1 of kernel image on should be loaded into struct
> > boot_params and examined. The end of setup header can be calculated as
> > follow:
> > 
> >         0x0202 + byte value at offset 0x0201
> 
> ... so we don't have to.

So, the problem here seems to be that there's never been widespread
compliance with this paragraph, but this patch assumes there has.  A
brief survey concludes:

grub 1 on bios - loads the kernel and edits the parameters it cares
                 about in place
grub 1 on efi - allocates a buffer (fails to clear it) and modifies
                the parameters it cares about, then copies it back
grub 2 on bios - clears the buffer, writes what it cares about
grub 2 on efi (using efi boot stub) - reads the buffer, modifies fields
        it cares about, passes the pointer to the boot stub
elilo - allocates a new buffer, copies the kernel structure in to it,
        allocates another buffer, clears it, copies the first structure
	in to it, frees the first buffer, modifies fields it cares about
	in the second buffer, clears some other fields in the second
	structure, and passes the pointer in when it calls the old entry
	point
	(It's possible that there's some newer version of elilo than 3.14,
	which I had handy, but I'm not going to do deeper research on a
	project that keeps a link to its CVS repo on the most obvious
	google result, lest I lose the will to live.)
syslinux - I'm just going to assume that your code matches the spec.

So it's certainly worth trying to find a better way to check this, but I
don't think this patch is it.  If we're going to enforce it, we have to
make sure that a bootloader that's conforming to what was de facto the
standard in 0x020b still works.  Otherwise we're just breaking
bootloaders for no reason, and that will end poorly.

I'd suggest we add a field for the bootloader to make a positive
declaration of what version it is using, and only check for the sentinel
if the field claims it's doing 0x020c or newer.

-- 
        Peter

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[H. Peter Anvin]

On 03/06/2013 08:55 AM, Peter Jones wrote:
> 
> So, the problem here seems to be that there's never been widespread
> compliance with this paragraph, but this patch assumes there has.  A
> brief survey concludes:
> 

No, this patch doesn't assume there is widespread compliance, it is
trying to address the bits that are not complied with.

> grub 1 on bios - loads the kernel and edits the parameters it cares
>                  about in place
> grub 1 on efi - allocates a buffer (fails to clear it) and modifies
>                 the parameters it cares about, then copies it back
> grub 2 on bios - clears the buffer, writes what it cares about

On BIOS, anything that invokes the 16-bit entry point will be correct,
because it is the 16-bit code that sets up struct boot_params.

> grub 2 on efi (using efi boot stub) - reads the buffer, modifies fields
>         it cares about, passes the pointer to the boot stub
> elilo - allocates a new buffer, copies the kernel structure in to it,
>         allocates another buffer, clears it, copies the first structure
> 	in to it, frees the first buffer, modifies fields it cares about
> 	in the second buffer, clears some other fields in the second
> 	structure, and passes the pointer in when it calls the old entry
> 	point
> 	(It's possible that there's some newer version of elilo than 3.14,
> 	which I had handy, but I'm not going to do deeper research on a
> 	project that keeps a link to its CVS repo on the most obvious
> 	google result, lest I lose the will to live.)
> syslinux - I'm just going to assume that your code matches the spec.
> 
> So it's certainly worth trying to find a better way to check this, but I
> don't think this patch is it.  If we're going to enforce it, we have to
> make sure that a bootloader that's conforming to what was de facto the
> standard in 0x020b still works.  Otherwise we're just breaking
> bootloaders for no reason, and that will end poorly.
> 
> I'd suggest we add a field for the bootloader to make a positive
> declaration of what version it is using, and only check for the sentinel
> if the field claims it's doing 0x020c or newer.

Except it doesn't quite work.  The problem is that these broken
bootloaders aren't just a matter of 2.11 vs 2.12, they are implicitly
assuming that the kernel image itself doesn't happen to contain anything
harmful in the fields that they don't bother initializing.  This would
be nice and good, except that the demands for the boot sector space is
fairly high and it gets very cantankerous to turn that into a minefield.

In fact, your suggestion is exactly equivalent to the sentinel, except
you want it to be pre-initialized with 0x20b instead of 0xffff.

As such, I don't really know anything better we can do other than:

1. detect the *properly working* case of the structure properly
   initialized;
2. doing legacy bootloader-specific clearing based on the bootloader ID
   if the sentinel triggers -- if you can think of better heuristics
   then that would be good;
3. try to get bootloaders switched from case #2 to case #1.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Revert commit 5dcd14ecd4 - breaks EFI boot with SLES11 elilo.efi

[Peter Jones]

On Wed, Mar 06, 2013 at 09:14:27AM -0800, H. Peter Anvin wrote:
> On 03/06/2013 08:55 AM, Peter Jones wrote:
> > 
> > So, the problem here seems to be that there's never been widespread
> > compliance with this paragraph, but this patch assumes there has.  A
> > brief survey concludes:
> 
> No, this patch doesn't assume there is widespread compliance, it is
> trying to address the bits that are not complied with.

Right, but that's basically every x86_64 UEFI machine ever deployed.

[lots trimmed]
> > So it's certainly worth trying to find a better way to check this, but I
> > don't think this patch is it.  If we're going to enforce it, we have to
> > make sure that a bootloader that's conforming to what was de facto the
> > standard in 0x020b still works.  Otherwise we're just breaking
> > bootloaders for no reason, and that will end poorly.
> > 
> > I'd suggest we add a field for the bootloader to make a positive
> > declaration of what version it is using, and only check for the sentinel
> > if the field claims it's doing 0x020c or newer.
> 
> Except it doesn't quite work.  The problem is that these broken
> bootloaders aren't just a matter of 2.11 vs 2.12, they are implicitly
> assuming that the kernel image itself doesn't happen to contain anything
> harmful in the fields that they don't bother initializing.  This would
> be nice and good, except that the demands for the boot sector space is
> fairly high and it gets very cantankerous to turn that into a minefield.

If your only objection is real estate, we can find a way to be clever
about what we do that uses already existing space.  For instance, write
back the version number that's supported in the version field, but
byte-swapped, so we can tell it changed (we don't anticipate ever
supporting protocol 0x20b from a kernel that advertises 0xb02, right?)

Just one example - we don't have to do this the exact way I said; we
just need a positive assertion from the bootloader to start doing
enforcement.  Versions would be nice, but they're not strictly required.

> In fact, your suggestion is exactly equivalent to the sentinel, except
> you want it to be pre-initialized with 0x20b instead of 0xffff.

No, I want the bootloader to communicate that it understands the boot
protocol revision is 0x020c, so we can /safely/ enter a world where
we're forbidding booting from an older bootloader.

> As such, I don't really know anything better we can do other than:
> 
> 1. detect the *properly working* case of the structure properly
>    initialized

Which is easy, but it doesn't seem to be anything anybody has ever
shipped on UEFI machines.

> 2. doing legacy bootloader-specific clearing based on the bootloader ID
>    if the sentinel triggers -- if you can think of better heuristics
>    then that would be good;

This heuristic is "all UEFI bootloaders anybody uses".  You can list
them individually, but it's the same as reverting the patch, just with
more code.

> 3. try to get bootloaders switched from case #2 to case #1.

And I'm for that, but I think we should delay enforcement until they've
got a way to express that.

-- 
        Peter