Re: [PATCH] x86/boot: Fix regression--secure boot info loss from bootparam sanitizing

Re: [PATCH] x86/boot: Fix regression--secure boot info loss from bootparam sanitizing

[John Hubbard]

On 9/1/19 8:38 AM, John S Gruber wrote:
> From: "John S. Gruber" <JohnSGruber@gmail.com>
> 
> commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
> else") now zeros the secure boot information passed by the boot loader or
> by the kernel's efi handover mechanism.
> 
> Include boot-params.secure_boot in the preserve field list.
> 
> Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
> ---
> 
> I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
> with signed kernels using the efi handoff protocol with grub. The kernel
> log message "Secure boot enabled" becomes "Secure boot could not be
> determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
> this field early but it is subsequently zeroed by the above referenced commit
> in the file arch/x86/include/asm/bootparam_utils.h
> 
> Applies to 5.3-rc6.
> 

Hi,

The fix itself looks good, so you can add:

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>

...but note that the commit description should get a few tweaks:

1. Your description above is actually well-suited for the commit log,
so please add that in. Especially the symptoms are desirable to have
on record.

2. This should Cc: stable@vger.kernel.org, because the whole thing
made it into -stable and those kernels need this fix.

3. Also need a Fixes tag:

Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")

thanks,
-- 
John Hubbard
NVIDIA

>   arch/x86/include/asm/bootparam_utils.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/include/asm/bootparam_utils.h
> b/arch/x86/include/asm/bootparam_utils.h
> index 9e5f3c7..981fe92 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
> *boot_params)
>   			BOOT_PARAM_PRESERVE(eddbuf_entries),
>   			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>   			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> +			BOOT_PARAM_PRESERVE(secure_boot),
>   			BOOT_PARAM_PRESERVE(hdr),
>   			BOOT_PARAM_PRESERVE(e820_table),
>   			BOOT_PARAM_PRESERVE(eddbuf),
> 

[PATCH V2] x86/boot: Fix regression--secure boot info loss from bootparam sanitizing

[John S Gruber]

From: "John S. Gruber" <JohnSGruber@gmail.com>

commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
else") now zeros the secure boot information passed by the boot loader or
by the kernel's efi handover mechanism.  Include boot-params.secure_boot
in the preserve field list.

I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
with signed kernels using the efi handoff protocol with grub. The kernel
log message "Secure boot enabled" becomes "Secure boot could not be
determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
this field early but it is subsequently zeroed by the above referenced
commit in the file arch/x86/include/asm/bootparam_utils.h

Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero
out everything else")
Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---

Adjusted the patch for John Hubbard's comments.

 arch/x86/include/asm/bootparam_utils.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/bootparam_utils.h
b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..981fe92 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
*boot_params)
 			BOOT_PARAM_PRESERVE(eddbuf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+			BOOT_PARAM_PRESERVE(secure_boot),
 			BOOT_PARAM_PRESERVE(hdr),
 			BOOT_PARAM_PRESERVE(e820_table),
 			BOOT_PARAM_PRESERVE(eddbuf),
-- 
2.7.4

Re: [PATCH V2] x86/boot: Fix regression--secure boot info loss from bootparam sanitizing

[Borislav Petkov]

On Mon, Sep 02, 2019 at 12:00:54AM +0200, John S Gruber wrote:
> From: "John S. Gruber" <JohnSGruber@gmail.com>
> 
> commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
> else") now zeros the secure boot information passed by the boot loader or
> by the kernel's efi handover mechanism.  Include boot-params.secure_boot
> in the preserve field list.
> 
> I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
> with signed kernels using the efi handoff protocol with grub. The kernel
> log message "Secure boot enabled" becomes "Secure boot could not be
> determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
> this field early but it is subsequently zeroed by the above referenced
> commit in the file arch/x86/include/asm/bootparam_utils.h
> 
> Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero
> out everything else")
> Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
> ---
> 
> Adjusted the patch for John Hubbard's comments.
> 
>  arch/x86/include/asm/bootparam_utils.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/include/asm/bootparam_utils.h
> b/arch/x86/include/asm/bootparam_utils.h
> index 9e5f3c7..981fe92 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
> *boot_params)

gmail has managed to chew this patch:

checking file arch/x86/include/asm/bootparam_utils.h
patch: **** malformed patch at line 48: *boot_params)

See: https://www.kernel.org/doc/html/latest/process/email-clients.html#gmail-web-gui

You might find a better client in there if you wanna send more patches
in the future.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip: x86/urgent] x86/boot: Preserve boot_params.secure_boot from sanitizing

[tip-bot2 for John S. Gruber]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     29d9a0b50736768f042752070e5cdf4e4d4c00df
Gitweb:        https://git.kernel.org/tip/29d9a0b50736768f042752070e5cdf4e4d4c00df
Author:        John S. Gruber <JohnSGruber@gmail.com>
AuthorDate:    Mon, 02 Sep 2019 00:00:54 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Mon, 02 Sep 2019 09:17:45 +02:00

x86/boot: Preserve boot_params.secure_boot from sanitizing

Commit

  a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")

now zeroes the secure boot setting information (enabled/disabled/...)
passed by the boot loader or by the kernel's EFI handover mechanism.

The problem manifests itself with signed kernels using the EFI handoff
protocol with grub and the kernel loses the information whether secure
boot is enabled in the firmware, i.e., the log message "Secure boot
enabled" becomes "Secure boot could not be determined".

efi_main() arch/x86/boot/compressed/eboot.c sets this field early but it
is subsequently zeroed by the above referenced commit.

Include boot_params.secure_boot in the preserve field list.

 [ bp: restructure commit message and massage. ]

Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: stable <stable@vger.kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/CAPotdmSPExAuQcy9iAHqX3js_fc4mMLQOTr5RBGvizyCOPcTQQ@mail.gmail.com
---
 arch/x86/include/asm/bootparam_utils.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..981fe92 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(eddbuf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+			BOOT_PARAM_PRESERVE(secure_boot),
 			BOOT_PARAM_PRESERVE(hdr),
 			BOOT_PARAM_PRESERVE(e820_table),
 			BOOT_PARAM_PRESERVE(eddbuf),

[PATCH] x86/boot: v4.4 stable and v4.9 stable boot failure due to dropped patch line

[John S Gruber]

This regards upstream commit a90118c445cc ("x86/boot: Save fields
explicitly, zero out everything else") application to linux-stable.

Its corresponding commits to the stable 4.4 and 4.9 trees didn't apply
correctly, probably due to a field name change (e820_table had been named
e820_map before 4.10).

On my desktop I'm unable to boot a signed kernel due to these commits.

Add e820_map (to replace e820_table) to the preserved fields so that the
E820 memory regions in boot_params can be accessed by the kernel after
boot_params has been sanitized.

Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
Fixes: 41664b97f46e ("x86/boot: Save fields explicitly, zero out everything else")
Fixes: 4e478cb2ccdd ("x86/boot: Save fields explicitly, zero out everything else")
Link: https://lore.kernel.org/lkml/20190731054627.5627-2-jhubbard@nvidia.com/
---

I tested stable 4.14.145, 4.19.74, and 5.2.16 successfully under the same
circumstances. Only 4.4 and 4.9 are affected by this dropped line.

 arch/x86/include/asm/bootparam_utils.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 0232b5a..588d8fb 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -71,6 +71,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
 			BOOT_PARAM_PRESERVE(hdr),
+			BOOT_PARAM_PRESERVE(e820_map),
 			BOOT_PARAM_PRESERVE(eddbuf),
 		};
 
-- 
2.7.4

Re: [PATCH] x86/boot: v4.4 stable and v4.9 stable boot failure due to dropped patch line

[John Hubbard]

On 9/20/19 6:06 PM, John S Gruber wrote:
> This regards upstream commit a90118c445cc ("x86/boot: Save fields
> explicitly, zero out everything else") application to linux-stable.
> 
> Its corresponding commits to the stable 4.4 and 4.9 trees didn't apply
> correctly, probably due to a field name change (e820_table had been named
> e820_map before 4.10).
> 
> On my desktop I'm unable to boot a signed kernel due to these commits.
> 
> Add e820_map (to replace e820_table) to the preserved fields so that the
> E820 memory regions in boot_params can be accessed by the kernel after
> boot_params has been sanitized.
> 
> Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
> Fixes: 41664b97f46e ("x86/boot: Save fields explicitly, zero out everything else")
> Fixes: 4e478cb2ccdd ("x86/boot: Save fields explicitly, zero out everything else")
> Link: https://lore.kernel.org/lkml/20190731054627.5627-2-jhubbard@nvidia.com/
> ---
> 
> I tested stable 4.14.145, 4.19.74, and 5.2.16 successfully under the same
> circumstances. Only 4.4 and 4.9 are affected by this dropped line.
> 
>  arch/x86/include/asm/bootparam_utils.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index 0232b5a..588d8fb 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -71,6 +71,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
>  			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>  			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>  			BOOT_PARAM_PRESERVE(hdr),
> +			BOOT_PARAM_PRESERVE(e820_map),
>  			BOOT_PARAM_PRESERVE(eddbuf),
>  		};
>  
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

Thanks for getting -stable figured out and fixed--this has not been smooth sailing!


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] x86/boot: v4.4 stable and v4.9 stable boot failure due to dropped patch line

[Greg Kroah-Hartman]

On Fri, Sep 20, 2019 at 06:38:23PM -0700, John Hubbard wrote:
> On 9/20/19 6:06 PM, John S Gruber wrote:
> > This regards upstream commit a90118c445cc ("x86/boot: Save fields
> > explicitly, zero out everything else") application to linux-stable.
> > 
> > Its corresponding commits to the stable 4.4 and 4.9 trees didn't apply
> > correctly, probably due to a field name change (e820_table had been named
> > e820_map before 4.10).
> > 
> > On my desktop I'm unable to boot a signed kernel due to these commits.
> > 
> > Add e820_map (to replace e820_table) to the preserved fields so that the
> > E820 memory regions in boot_params can be accessed by the kernel after
> > boot_params has been sanitized.
> > 
> > Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
> > Fixes: 41664b97f46e ("x86/boot: Save fields explicitly, zero out everything else")
> > Fixes: 4e478cb2ccdd ("x86/boot: Save fields explicitly, zero out everything else")
> > Link: https://lore.kernel.org/lkml/20190731054627.5627-2-jhubbard@nvidia.com/
> > ---
> > 
> > I tested stable 4.14.145, 4.19.74, and 5.2.16 successfully under the same
> > circumstances. Only 4.4 and 4.9 are affected by this dropped line.
> > 
> >  arch/x86/include/asm/bootparam_utils.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> > index 0232b5a..588d8fb 100644
> > --- a/arch/x86/include/asm/bootparam_utils.h
> > +++ b/arch/x86/include/asm/bootparam_utils.h
> > @@ -71,6 +71,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
> >  			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> >  			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> >  			BOOT_PARAM_PRESERVE(hdr),
> > +			BOOT_PARAM_PRESERVE(e820_map),
> >  			BOOT_PARAM_PRESERVE(eddbuf),
> >  		};
> >  
> > 
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> 
> Thanks for getting -stable figured out and fixed--this has not been smooth sailing!

This change is already in the 4.4.y-rc and 4.9.y-rc releases that went
out two days ago for review.  So this should be fixed in the next
release.  If not, please let me know.

thanks,

greg k-h