Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Matt Fleming]

On Wed, 03 Feb, at 10:53:33PM, Matt Fleming wrote:
> On Thu, 04 Feb, at 05:42:00AM, Dave Young wrote:
> > 
> > On 01/27/16 at 07:20pm, Dave Young wrote:
> > > For kexec reboot the bgrt image address could contains random data because
> > > we have freed boot service areas in 1st kernel boot phase. One possible
> > > result is kmalloc fail in efi_bgrt_init due to large random image size.
> > > 
> > > So change efi_late_init to avoid efi_bgrt_init in case kexec boot.
> > > 
> > > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  arch/x86/platform/efi/efi.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > > +++ linux-x86/arch/x86/platform/efi/efi.c
> > > @@ -531,7 +531,8 @@ void __init efi_init(void)
> > >  
> > >  void __init efi_late_init(void)
> > >  {
> > > -	efi_bgrt_init();
> > > +	if (!efi_setup)
> > > +		efi_bgrt_init();
> > >  }
> > >  
> > >  void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
> > 
> > Matt, opinions about this patch?
> 
> Yeah, I'm not happy seeing efi_setup escaping into even more places,
> nor am I happy to see more code paths introduced where kexec boot is
> special-cased.
> 
> I'll reply with more details tomorrow.

OK, let me expand upon that rather terse feedback. 

This patch highlights a general problem I see in the EFI code which is
that we're continuously increasing the number of execution paths
through the boot code. This makes it increasingly difficult to modify
the code without introducing bugs and regressions.

I was bitten by this recently with the EFI separate page table rework,
which led to commit 753b11ef8e92 ("x86/efi: Setup separate EFI page
tables in kexec paths"), i.e I forgot to update the special kexec
virtual mapping function.

We should be reducing the use of 'efi_setup', not adding more uses.

As an aside, I've always had a problem with using 'efi_setup' to
indicate when we've been booted via kexec. If a developer with no
prior knowledge reads those if conditions they are going to have zero
clue what the code means. 

Now, specifically for the issue you've raised, would it not make more
sense for kexec to build its own ACPI tables and omit those entries
that are not valid, e.g. BGRT? I can imagine that the BGRT driver
won't be the only driver with this problem. Let's re-use the existing
error paths that handle missing/invalid tables.

Fundamentally I don't think there should be a discernible difference
between "Booted via kexec" and "That ACPI table does not exist".

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Dave Young]

Hi, Matt

Thanks for the feedback.

On 02/04/16 at 10:03am, Matt Fleming wrote:
> On Wed, 03 Feb, at 10:53:33PM, Matt Fleming wrote:
> > On Thu, 04 Feb, at 05:42:00AM, Dave Young wrote:
> > > 
> > > On 01/27/16 at 07:20pm, Dave Young wrote:
> > > > For kexec reboot the bgrt image address could contains random data because
> > > > we have freed boot service areas in 1st kernel boot phase. One possible
> > > > result is kmalloc fail in efi_bgrt_init due to large random image size.
> > > > 
> > > > So change efi_late_init to avoid efi_bgrt_init in case kexec boot.
> > > > 
> > > > Signed-off-by: Dave Young <dyoung@redhat.com>
> > > > ---
> > > >  arch/x86/platform/efi/efi.c |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > > > +++ linux-x86/arch/x86/platform/efi/efi.c
> > > > @@ -531,7 +531,8 @@ void __init efi_init(void)
> > > >  
> > > >  void __init efi_late_init(void)
> > > >  {
> > > > -	efi_bgrt_init();
> > > > +	if (!efi_setup)
> > > > +		efi_bgrt_init();
> > > >  }
> > > >  
> > > >  void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
> > > 
> > > Matt, opinions about this patch?
> > 
> > Yeah, I'm not happy seeing efi_setup escaping into even more places,
> > nor am I happy to see more code paths introduced where kexec boot is
> > special-cased.
> > 
> > I'll reply with more details tomorrow.
> 
> OK, let me expand upon that rather terse feedback. 
> 
> This patch highlights a general problem I see in the EFI code which is
> that we're continuously increasing the number of execution paths
> through the boot code. This makes it increasingly difficult to modify
> the code without introducing bugs and regressions.
> 
> I was bitten by this recently with the EFI separate page table rework,
> which led to commit 753b11ef8e92 ("x86/efi: Setup separate EFI page
> tables in kexec paths"), i.e I forgot to update the special kexec
> virtual mapping function.
> 
> We should be reducing the use of 'efi_setup', not adding more uses.

I agree with you the less special case the better.

> 
> As an aside, I've always had a problem with using 'efi_setup' to
> indicate when we've been booted via kexec. If a developer with no
> prior knowledge reads those if conditions they are going to have zero
> clue what the code means. 

Consider the original code path, maybe change it to efi_kexec_setup will
be better to remind people? Or something else like a wraper function with
similar name..

> 
> Now, specifically for the issue you've raised, would it not make more
> sense for kexec to build its own ACPI tables and omit those entries
> that are not valid, e.g. BGRT? I can imagine that the BGRT driver
> won't be the only driver with this problem. Let's re-use the existing
> error paths that handle missing/invalid tables.
> 
> Fundamentally I don't think there should be a discernible difference
> between "Booted via kexec" and "That ACPI table does not exist".

For building ACPI tables we need do it in kernel instead of kexec-tools
because of kexec_file_load for secure boot case so we still need a conditional
code path for kexec..

Also I'm not sure how to rebuild ACPI tables, it is easy or hard. Let me
checking the detail and think more about it.

Thanks a lot
Dave

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Matt Fleming]

On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> 
> Consider the original code path, maybe change it to efi_kexec_setup will
> be better to remind people? Or something else like a wraper function with
> similar name..
 
Possibly. I had considered adding a new efi_enabled() bit for
KEXEC_BOOT, but I'm worried that'll just encourage more uses.

The best approach is going to be to see whether we can reduce the uses
of efi_setup and the associated special code. Once we've completed
that exercise, we can think about the best name for this variable.

> For building ACPI tables we need do it in kernel instead of kexec-tools
> because of kexec_file_load for secure boot case so we still need a conditional
> code path for kexec..

Note that it may not be necessary to build any ACPI tables at all,
provided that things like acpi_get_table() fail gracefully for kexec.
I'm assuming that's the problem that you discovered when writing this
patch.

And yes, I don't expect you can build the ACPI table from userspace,
but it should at least be possible to do it in setup_boot_parameters()
or so when you setup the EFI table pointers (efi.config_tables), etc.
I think that would be a natural home for this feature.

> Also I'm not sure how to rebuild ACPI tables, it is easy or hard. Let me
> checking the detail and think more about it.

Thanks.

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Dave Young]

On 02/04/16 at 11:56am, Matt Fleming wrote:
> On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> > 
> > Consider the original code path, maybe change it to efi_kexec_setup will
> > be better to remind people? Or something else like a wraper function with
> > similar name..
>  
> Possibly. I had considered adding a new efi_enabled() bit for
> KEXEC_BOOT, but I'm worried that'll just encourage more uses.
> 
> The best approach is going to be to see whether we can reduce the uses
> of efi_setup and the associated special code. Once we've completed
> that exercise, we can think about the best name for this variable.

Ok, thanks.

> 
> > For building ACPI tables we need do it in kernel instead of kexec-tools
> > because of kexec_file_load for secure boot case so we still need a conditional
> > code path for kexec..
> 
> Note that it may not be necessary to build any ACPI tables at all,
> provided that things like acpi_get_table() fail gracefully for kexec.
> I'm assuming that's the problem that you discovered when writing this
> patch.
> 
> And yes, I don't expect you can build the ACPI table from userspace,
> but it should at least be possible to do it in setup_boot_parameters()
> or so when you setup the EFI table pointers (efi.config_tables), etc.
> I think that would be a natural home for this feature.

Thing is we support both kexec_load and kexec_file_load, if we do something
in kernel loader we will need do same in userspace kexec-tools as well.

Another way is we probably can retain the boot service areas for kexec
boot, but yes it is another special handling for kexec :(. Is this way
better to you?

Thanks
Dave

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Matt Fleming]

On Fri, 05 Feb, at 08:41:15AM, Dave Young wrote:
> On 02/04/16 at 11:56am, Matt Fleming wrote:
> > On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> > > 
> > > Consider the original code path, maybe change it to efi_kexec_setup will
> > > be better to remind people? Or something else like a wraper function with
> > > similar name..
> >  
> > Possibly. I had considered adding a new efi_enabled() bit for
> > KEXEC_BOOT, but I'm worried that'll just encourage more uses.
> > 
> > The best approach is going to be to see whether we can reduce the uses
> > of efi_setup and the associated special code. Once we've completed
> > that exercise, we can think about the best name for this variable.
> 
> Ok, thanks.
> 
> > 
> > > For building ACPI tables we need do it in kernel instead of kexec-tools
> > > because of kexec_file_load for secure boot case so we still need a conditional
> > > code path for kexec..
> > 
> > Note that it may not be necessary to build any ACPI tables at all,
> > provided that things like acpi_get_table() fail gracefully for kexec.
> > I'm assuming that's the problem that you discovered when writing this
> > patch.
> > 
> > And yes, I don't expect you can build the ACPI table from userspace,
> > but it should at least be possible to do it in setup_boot_parameters()
> > or so when you setup the EFI table pointers (efi.config_tables), etc.
> > I think that would be a natural home for this feature.
> 
> Thing is we support both kexec_load and kexec_file_load, if we do something
> in kernel loader we will need do same in userspace kexec-tools as well.
 
Actually, on thinking about it a little bit more, any changes we make
would have to be on the second kernel side, because otherwise you end
up with incompatibilities between kernel versions.

For example, changing the data we pass in the SETUP_EFI chain is a bad
idea becuase you then have to care about exactly which kernel version
you kexec loaded.

> Another way is we probably can retain the boot service areas for kexec
> boot, but yes it is another special handling for kexec :(. Is this way
> better to you?

Unfortunately retaining Boot Services areas is infeasible because they
can be *really* large, i.e. multiple gigabytes in size.

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Dave Young]

On 02/11/16 at 04:09pm, Matt Fleming wrote:
> On Fri, 05 Feb, at 08:41:15AM, Dave Young wrote:
> > On 02/04/16 at 11:56am, Matt Fleming wrote:
> > > On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote:
> > > > 
> > > > Consider the original code path, maybe change it to efi_kexec_setup will
> > > > be better to remind people? Or something else like a wraper function with
> > > > similar name..
> > >  
> > > Possibly. I had considered adding a new efi_enabled() bit for
> > > KEXEC_BOOT, but I'm worried that'll just encourage more uses.
> > > 
> > > The best approach is going to be to see whether we can reduce the uses
> > > of efi_setup and the associated special code. Once we've completed
> > > that exercise, we can think about the best name for this variable.
> > 
> > Ok, thanks.
> > 
> > > 
> > > > For building ACPI tables we need do it in kernel instead of kexec-tools
> > > > because of kexec_file_load for secure boot case so we still need a conditional
> > > > code path for kexec..
> > > 
> > > Note that it may not be necessary to build any ACPI tables at all,
> > > provided that things like acpi_get_table() fail gracefully for kexec.
> > > I'm assuming that's the problem that you discovered when writing this
> > > patch.
> > > 
> > > And yes, I don't expect you can build the ACPI table from userspace,
> > > but it should at least be possible to do it in setup_boot_parameters()
> > > or so when you setup the EFI table pointers (efi.config_tables), etc.
> > > I think that would be a natural home for this feature.
> > 
> > Thing is we support both kexec_load and kexec_file_load, if we do something
> > in kernel loader we will need do same in userspace kexec-tools as well.
>  
> Actually, on thinking about it a little bit more, any changes we make
> would have to be on the second kernel side, because otherwise you end
> up with incompatibilities between kernel versions.

Hmm, agreed.

> 
> For example, changing the data we pass in the SETUP_EFI chain is a bad
> idea becuase you then have to care about exactly which kernel version
> you kexec loaded.
> 
> > Another way is we probably can retain the boot service areas for kexec
> > boot, but yes it is another special handling for kexec :(. Is this way
> > better to you?
> 
> Unfortunately retaining Boot Services areas is infeasible because they
> can be *really* large, i.e. multiple gigabytes in size.

Yes, rethink about this issue, how about something like below:
---

For kexec reboot the bgrt image address could contains random data because
we have freed boot service areas in 1st kernel boot phase. One possible
result is kmalloc fail in efi_bgrt_init due to large random image size.

bgrt table finished it's task after copying bgrt image thus let's
change the table status to be invalid at the end of efi_bgrt_init. 

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 arch/x86/platform/efi/efi-bgrt.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
+++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
@@ -107,4 +107,10 @@ void __init efi_bgrt_init(void)
 	memcpy_fromio(bgrt_image, image, bgrt_image_size);
 	if (ioremapped)
 		early_iounmap(image, bmp_header.size);
+
+	/*
+	 * Invalidate bgrt table because kexec reboot will access the bgrt image
+	 * which stays in freed boot service area.
+	 */
+	bgrt_tab->status &= 0xfe;
 }

Re: [PATCH] x86/efi: skip bgrt init for kexec reboot

[Matt Fleming]

On Fri, 12 Feb, at 08:45:42PM, Dave Young wrote:
> 
> Yes, rethink about this issue, how about something like below:

Let's put a pin this idea. I've got another approach that I want to
explore a little first. I'll send the patches out soon.