Re: [PATCH v3] tpm: only set efi_tpm_final_log_size after successful event log parsing

2019-09-25 Thread Ard Biesheuvel
On Wed, 25 Sep 2019 at 19:27, Jerry Snitselaar  wrote:
>
> If __calc_tpm2_event_size fails to parse an event it will return 0,
> resulting tpm2_calc_event_log_size returning -1. Currently there is
> no check of this return value, and efi_tpm_final_log_size can end up
> being set to this negative value resulting in a panic like the
> the one given below.
>
> Also __calc_tpm2_event_size returns a size of 0 when it fails
> to parse an event, so update function documentation to reflect this.
>
...
>
> The root cause of the issue that caused the failure of event parsing
> in this case is resolved by Peter Jone's patchset dealing with large
> event logs where crossing over a page boundary causes the page with
> the event count to be unmapped.
>
> Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
> Cc: linux-efi@vger.kernel.org
> Cc: linux-integr...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Cc: Matthew Garrett 
> Cc: Ard Biesheuvel 
> Cc: Jarkko Sakkinen 
> Signed-off-by: Jerry Snitselaar 

Thanks Jerry, I have queued this in the efi/urgent branch.


> ---
> v3: rebase on top of Peter Jone's patchset
> v2: added FW_BUG to pr_err, and renamed label to out_calc.
> Updated doc comment for __calc_tpm2_event_size.
>
>  drivers/firmware/efi/tpm.c   | 9 -
>  include/linux/tpm_eventlog.h | 2 +-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index b9ae5c6f9b9c..703469c1ab8e 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -85,11 +85,18 @@ int __init efi_tpm_eventlog_init(void)
> final_tbl->nr_events,
> log_tbl->log);
> }
> +
> +   if (tbl_size < 0) {
> +   pr_err(FW_BUG "Failed to parse event in TPM Final Events 
> Log\n");
> +   goto out_calc;
> +   }
> +
> memblock_reserve((unsigned long)final_tbl,
>  tbl_size + sizeof(*final_tbl));
> -   early_memunmap(final_tbl, sizeof(*final_tbl));
> efi_tpm_final_log_size = tbl_size;
>
> +out_calc:
> +   early_memunmap(final_tbl, sizeof(*final_tbl));
>  out:
> early_memunmap(log_tbl, sizeof(*log_tbl));
> return ret;
> diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> index 12584b69a3f3..2dfdd63ac034 100644
> --- a/include/linux/tpm_eventlog.h
> +++ b/include/linux/tpm_eventlog.h
> @@ -152,7 +152,7 @@ struct tcg_algorithm_info {
>   * total. Once we've done this we know the offset of the data length field,
>   * and can calculate the total size of the event.
>   *
> - * Return: size of the event on success, <0 on failure
> + * Return: size of the event on success, 0 on failure
>   */
>
>  static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> --
> 2.23.0
>


Re: [PATCH v2] x86, efi: never relocate kernel below lowest acceptable address

2019-09-25 Thread Kairui Song
On Wed, Sep 25, 2019 at 11:25 PM Ard Biesheuvel
 wrote:
>
> On Thu, 19 Sep 2019 at 18:06, Kairui Song  wrote:
> >
> > Currently, kernel fails to boot on some HyperV VMs when using EFI.
> > And it's a potential issue on all platforms.
> >
> > It's caused a broken kernel relocation on EFI systems, when below three
> > conditions are met:
> >
> > 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
> >by the loader.
> > 2. There isn't enough room to contain the kernel, starting from the
> >default load address (eg. something else occupied part the region).
> > 3. In the memmap provided by EFI firmware, there is a memory region
> >starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
> >kernel.
> >
> > Efi stub will perform a kernel relocation when condition 1 is met. But
> > due to condition 2, efi stub can't relocate kernel to the preferred
> > address, so it fallback to query and alloc from EFI firmware for lowest
> > usable memory region.
> >
> > It's incorrect to use the lowest memory address. In later stage, kernel
> > will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> > but efi stub will end up relocating kernel below it.
> >
> > Then before the kernel decompressing. Kernel will do another relocation
> > to address not lower than LOAD_PHYSICAL_ADDR, this time the relocate will
> > over write the blockage at the default load address, which efi stub tried
> > to avoid, and lead to unexpected behavior. Beside, the memory region it
> > writes to is not allocated from EFI firmware, which is also wrong.
> >
> > To fix it, just don't let efi stub relocate the kernel to any address
> > lower than lowest acceptable address.
> >
> > Signed-off-by: Kairui Song 
> >
>
> Hello Kairui,
>
> This patch looks correct to me, but it needs an ack from the x86
> maintainers, since the rules around LOAD_PHYSICAL_ADDR are specific to
> the x86 architecture.
>
>

Thanks for the review, Ard.

Can any x86 maintainer help provide some review?

-- 
Best Regards,
Kairui Song


[PATCH v3] tpm: only set efi_tpm_final_log_size after successful event log parsing

2019-09-25 Thread Jerry Snitselaar
If __calc_tpm2_event_size fails to parse an event it will return 0,
resulting tpm2_calc_event_log_size returning -1. Currently there is
no check of this return value, and efi_tpm_final_log_size can end up
being set to this negative value resulting in a panic like the
the one given below.

Also __calc_tpm2_event_size returns a size of 0 when it fails
to parse an event, so update function documentation to reflect this.

[0.774340] BUG: unable to handle page fault for address: bc8fc00866ad
[0.774788] #PF: supervisor read access in kernel mode
[0.774788] #PF: error_code(0x) - not-present page
[0.774788] PGD 107d36067 P4D 107d36067 PUD 107d37067 PMD 107d38067 PTE 0
[0.774788] Oops:  [#1] SMP PTI
[0.774788] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
5.3.0-0.rc2.1.elrdy.x86_64 #1
[0.774788] Hardware name: LENOVO 20HGS22D0W/20HGS22D0W, BIOS N1WET51W (1.30 
) 09/14/2018
[0.774788] RIP: 0010:memcpy_erms+0x6/0x10
[0.774788] Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 
83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1  a4 c3 
0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
[0.774788] RSP: :bc8fc0073b30 EFLAGS: 00010286
[0.774788] RAX: 9b1fc7c5b367 RBX: 9b1fc839 RCX: e962
[0.774788] RDX: e962 RSI: bc8fc00866ad RDI: 9b1fc7c5b367
[0.774788] RBP: 9b1c10ca7018 R08: bc8fc0085fff R09: 8063
[0.774788] R10: 1000 R11: 000fffe0 R12: 3367
[0.774788] R13: 9b1fcc47c010 R14: bc8fc0085000 R15: 0002
[0.774788] FS:  () GS:9b1fce20() 
knlGS:
[0.774788] CS:  0010 DS:  ES:  CR0: 80050033
[0.774788] CR2: bc8fc00866ad CR3: 00029f60a001 CR4: 003606f0
[0.774788] DR0:  DR1:  DR2: 
[0.774788] DR3:  DR6: fffe0ff0 DR7: 0400
[0.774788] Call Trace:
[0.774788]  tpm_read_log_efi+0x156/0x1a0
[0.774788]  tpm_bios_log_setup+0xc8/0x190
[0.774788]  tpm_chip_register+0x50/0x1c0
[0.774788]  tpm_tis_core_init.cold.9+0x28c/0x466
[0.774788]  tpm_tis_plat_probe+0xcc/0xea
[0.774788]  platform_drv_probe+0x35/0x80
[0.774788]  really_probe+0xef/0x390
[0.774788]  driver_probe_device+0xb4/0x100
[0.774788]  device_driver_attach+0x4f/0x60
[0.774788]  __driver_attach+0x86/0x140
[0.774788]  ? device_driver_attach+0x60/0x60
[0.774788]  bus_for_each_dev+0x76/0xc0
[0.774788]  ? klist_add_tail+0x3b/0x70
[0.774788]  bus_add_driver+0x14a/0x1e0
[0.774788]  ? tpm_init+0xea/0xea
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  driver_register+0x6b/0xb0
[0.774788]  ? tpm_init+0xea/0xea
[0.774788]  init_tis+0x86/0xd8
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  ? driver_register+0x94/0xb0
[0.774788]  do_one_initcall+0x46/0x1e4
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  kernel_init_freeable+0x199/0x242
[0.774788]  ? rest_init+0xaa/0xaa
[0.774788]  kernel_init+0xa/0x106
[0.774788]  ret_from_fork+0x35/0x40
[0.774788] Modules linked in:
[0.774788] CR2: bc8fc00866ad
[0.774788] ---[ end trace 42930799f8d6eaea ]---
[0.774788] RIP: 0010:memcpy_erms+0x6/0x10
[0.774788] Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 
83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1  a4 c3 
0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
[0.774788] RSP: :bc8fc0073b30 EFLAGS: 00010286
[0.774788] RAX: 9b1fc7c5b367 RBX: 9b1fc839 RCX: e962
[0.774788] RDX: e962 RSI: bc8fc00866ad RDI: 9b1fc7c5b367
[0.774788] RBP: 9b1c10ca7018 R08: bc8fc0085fff R09: 8063
[0.774788] R10: 1000 R11: 000fffe0 R12: 3367
[0.774788] R13: 9b1fcc47c010 R14: bc8fc0085000 R15: 0002
[0.774788] FS:  () GS:9b1fce20() 
knlGS:
[0.774788] CS:  0010 DS:  ES:  CR0: 80050033
[0.774788] CR2: bc8fc00866ad CR3: 00029f60a001 CR4: 003606f0
[0.774788] DR0:  DR1:  DR2: 
[0.774788] DR3:  DR6: fffe0ff0 DR7: 0400
[0.774788] Kernel panic - not syncing: Fatal exception
[0.774788] Kernel Offset: 0x1d00 from 0x8100 (relocation 
range: 0x8000-0xbfff)
[0.774788] ---[ end Kernel panic - not syncing: Fatal exception ]---

The root cause of the issue that caused the failure of event parsing
in this case is resolved by Peter Jone's patchset dealing with large
event logs where crossing over a page boundary causes the page with
the event count to be unmapped.

Fixes: c46f3405692de 

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

2019-09-25 Thread Jerry Snitselaar

On Wed Sep 25 19, Jerry Snitselaar wrote:

On Wed Sep 25 19, Jarkko Sakkinen wrote:

On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:

On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
 wrote:


From: Peter Jones 

Some machines generate a lot of event log entries.  When we're
iterating over them, the code removes the old mapping and adds a
new one, so once we cross the page boundary we're unmapping the page
with the count on it.  Hilarity ensues.

This patch keeps the info from the header in local variables so we don't
need to access that page again or keep track of if it's mapped.

Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
Cc: linux-efi@vger.kernel.org
Cc: linux-integr...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Peter Jones 
Tested-by: Lyude Paul 
Reviewed-by: Jarkko Sakkinen 
Acked-by: Matthew Garrett 
Acked-by: Ard Biesheuvel 
Signed-off-by: Jarkko Sakkinen 


Thanks Jarkko.

Shall I take these through the EFI tree?


Would be great, if you could because I already sent one PR with fixes for
v5.4-rc1 yesterday.

/Jarkko


My patch collides with this, so I will submit a v3 that applies on top of
these once I've run a test with all 3 applied on this t480s.


Tested with Peter's patches, and that was the root cause on this 480s.

I think there should still be a check for tbl_size to make sure we
aren't sticking -1 into efi_tpm_final_log_size though, which will be
the case right now if it fails to parse an event.


Re: [PATCH] efi: don't iterate over EFI vars pointlessly if no SSDT override was specified

2019-09-25 Thread Ard Biesheuvel
On Thu, 19 Sep 2019 at 20:01, Scott Talbert  wrote:
>
> On Thu, 12 Sep 2019, Ard Biesheuvel wrote:
>
> >>> The kernel command line option efivar_ssdt= allows a EFI variable name
> >>> to be specified which contains an ACPI SSDT table that will be loaded
> >>> into memory by the OS.
> >>>
> >>> Currently, that code will always iterate over the EFI variables and
> >>> compare each name with the provided name, even if the command line
> >>> option wasn't set to begin with.
> >>>
> >>> So bail early when no variable name was provided.
> >>>
> >>> Cc: Scott Talbert 
> >>> Signed-off-by: Ard Biesheuvel 
> >>> ---
> >>> drivers/firmware/efi/efi.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> >>> index ad3b1f4866b3..8f020827cdd3 100644
> >>> --- a/drivers/firmware/efi/efi.c
> >>> +++ b/drivers/firmware/efi/efi.c
> >>> @@ -282,6 +282,9 @@ static __init int efivar_ssdt_load(void)
> >>>   void *data;
> >>>   int ret;
> >>>
> >>> + if (!efivar_ssdt[0])
> >>> + return 0;
> >>> +
> >>>   ret = efivar_init(efivar_ssdt_iter, , true, );
> >>>
> >>>   list_for_each_entry_safe(entry, aux, , list) {
> >>
> >> Thanks for the quick fix!
> >>
> >> I can confirm this fixes booting on my Mac Pro 2012 system when applied to
> >> 5.3-rc7.
> >>
> >> Whenever this makes it in, if it could be targeted for the stable kernels
> >> as well, that would be appreciated.
> >>
> >
> > I'll send it out as a fix with a cc to -stable.
>
> Hi - just a quick reminder on this as I don't see it in Linus' tree yet.
> Not that I need it urgently, but just want to make sure it isn't
> forgotten.
>

Hi Scott,

This should get sent out in the next couple of day. It usually takes
another week or so after that for changes to make it into Linus's
tree.


Re: [PATCH 1/2] efi: Add efi_memmap_free() to free EFI memory map

2019-09-25 Thread Ard Biesheuvel
On Wed, 25 Sep 2019 at 11:17, Yunfeng Ye  wrote:
>
> In efi_fake_memmap(), the commit 20b1e22d01a4 ("x86/efi: Don't allocate
> memmap through memblock after mm_init()") replace memblock_alloc() with
> efi_memmap_alloc(), but there is no matching modification of
> memblock_free() when early_memremap() fail.
>
> Fix this by adding efi_memmap_free() to instead of memblock_free().
>
> Fixes: 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after 
> mm_init()")
> Signed-off-by: Yunfeng Ye 

What happens if you try to call efi_memmap_free() /after/ slab has
become available on an allocation that was created before?

> ---
>  drivers/firmware/efi/fake_mem.c |  2 +-
>  drivers/firmware/efi/memmap.c   | 34 ++
>  include/linux/efi.h |  1 +
>  3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
> index 9501edc..c2f69f6 100644
> --- a/drivers/firmware/efi/fake_mem.c
> +++ b/drivers/firmware/efi/fake_mem.c
> @@ -65,7 +65,7 @@ void __init efi_fake_memmap(void)
> new_memmap = early_memremap(new_memmap_phy,
> efi.memmap.desc_size * new_nr_map);
> if (!new_memmap) {
> -   memblock_free(new_memmap_phy, efi.memmap.desc_size * 
> new_nr_map);
> +   efi_memmap_free(new_memmap_phy, new_nr_map);
> return;
> }
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 38b686c..35dc189 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -18,6 +18,11 @@ static phys_addr_t __init 
> __efi_memmap_alloc_early(unsigned long size)
> return memblock_phys_alloc(size, SMP_CACHE_BYTES);
>  }
>
> +static void __init __efi_memmap_free_early(phys_addr_t addr, unsigned long 
> size)
> +{
> +   memblock_free(addr, size);
> +}
> +
>  static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
>  {
> unsigned int order = get_order(size);
> @@ -29,6 +34,15 @@ static phys_addr_t __init __efi_memmap_alloc_late(unsigned 
> long size)
> return PFN_PHYS(page_to_pfn(p));
>  }
>
> +static void __init __efi_memmap_free_late(phys_addr_t addr, unsigned long 
> size)
> +{
> +   unsigned int order = get_order(size);
> +   struct page *p = pfn_to_page(PHYS_PFN(addr));
> +
> +   if (p)
> +   __free_pages(p, order);
> +}
> +
>  /**
>   * efi_memmap_alloc - Allocate memory for the EFI memory map
>   * @num_entries: Number of entries in the allocated map.
> @@ -50,6 +64,26 @@ phys_addr_t __init efi_memmap_alloc(unsigned int 
> num_entries)
>  }
>
>  /**
> + * efi_memmap_free - Free memory for the EFI memory map
> + * @addr: Physical address of the EFI memory map to be freed.
> + * @num_entries: Number of the EFI memory map entries.
> + *
> + * Depending on whether mm_init() has already been invoked or not,
> + * either memblock or "normal" page free is used.
> + */
> +void __init efi_memmap_free(phys_addr_t addr, unsigned int num_entries)
> +{
> +   unsigned long size = num_entries * efi.memmap.desc_size;
> +
> +   if (slab_is_available()) {
> +   __efi_memmap_free_late(addr, size);
> +
> +   return;
> +   }
> +   __efi_memmap_free_early(addr, size);
> +}
> +
> +/**
>   * __efi_memmap_init - Common code for mapping the EFI memory map
>   * @data: EFI memory map data
>   * @late: Use early or late mapping function?
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bd38370..8bb741a 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1057,6 +1057,7 @@ static inline efi_status_t efi_query_variable_store(u32 
> attributes,
>  extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
>
>  extern phys_addr_t __init efi_memmap_alloc(unsigned int num_entries);
> +extern void __init efi_memmap_free(phys_addr_t addr, unsigned int 
> num_entries);
>  extern int __init efi_memmap_init_early(struct efi_memory_map_data *data);
>  extern int __init efi_memmap_init_late(phys_addr_t addr, unsigned long size);
>  extern void __init efi_memmap_unmap(void);
> --
> 1.8.3.1
>


Re: [PATCH v2] x86, efi: never relocate kernel below lowest acceptable address

2019-09-25 Thread Ard Biesheuvel
On Thu, 19 Sep 2019 at 18:06, Kairui Song  wrote:
>
> Currently, kernel fails to boot on some HyperV VMs when using EFI.
> And it's a potential issue on all platforms.
>
> It's caused a broken kernel relocation on EFI systems, when below three
> conditions are met:
>
> 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
>by the loader.
> 2. There isn't enough room to contain the kernel, starting from the
>default load address (eg. something else occupied part the region).
> 3. In the memmap provided by EFI firmware, there is a memory region
>starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
>kernel.
>
> Efi stub will perform a kernel relocation when condition 1 is met. But
> due to condition 2, efi stub can't relocate kernel to the preferred
> address, so it fallback to query and alloc from EFI firmware for lowest
> usable memory region.
>
> It's incorrect to use the lowest memory address. In later stage, kernel
> will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> but efi stub will end up relocating kernel below it.
>
> Then before the kernel decompressing. Kernel will do another relocation
> to address not lower than LOAD_PHYSICAL_ADDR, this time the relocate will
> over write the blockage at the default load address, which efi stub tried
> to avoid, and lead to unexpected behavior. Beside, the memory region it
> writes to is not allocated from EFI firmware, which is also wrong.
>
> To fix it, just don't let efi stub relocate the kernel to any address
> lower than lowest acceptable address.
>
> Signed-off-by: Kairui Song 
>

Hello Kairui,

This patch looks correct to me, but it needs an ack from the x86
maintainers, since the rules around LOAD_PHYSICAL_ADDR are specific to
the x86 architecture.


> ---
>
> Update from V1:
>  - Redo the commit message.
>
>  arch/x86/boot/compressed/eboot.c   |  8 +---
>  drivers/firmware/efi/libstub/arm32-stub.c  |  2 +-
>  drivers/firmware/efi/libstub/arm64-stub.c  |  2 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 12 
>  include/linux/efi.h|  5 +++--
>  5 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index 936bdb924ec2..8207e8aa297e 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "../string.h"
>  #include "eboot.h"
> @@ -432,7 +433,7 @@ struct boot_params *make_boot_params(struct efi_config *c)
> }
>
> status = efi_low_alloc(sys_table, 0x4000, 1,
> -  (unsigned long *)_params);
> +  (unsigned long *)_params, 0);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to allocate lowmem for boot 
> params\n");
> return NULL;
> @@ -817,7 +818,7 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
>
> gdt->size = 0x800;
> status = efi_low_alloc(sys_table, gdt->size, 8,
> -  (unsigned long *)>address);
> +  (unsigned long *)>address, 0);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to allocate memory for 
> 'gdt'\n");
> goto fail;
> @@ -842,7 +843,8 @@ efi_main(struct efi_config *c, struct boot_params 
> *boot_params)
> status = efi_relocate_kernel(sys_table, _addr,
>  hdr->init_size, hdr->init_size,
>  hdr->pref_address,
> -hdr->kernel_alignment);
> +hdr->kernel_alignment,
> +LOAD_PHYSICAL_ADDR);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "efi_relocate_kernel() 
> failed!\n");
> goto fail;
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c 
> b/drivers/firmware/efi/libstub/arm32-stub.c
> index e8f7aefb6813..bf6f954d6afe 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -220,7 +220,7 @@ efi_status_t handle_kernel_image(efi_system_table_t 
> *sys_table,
> *image_size = image->image_size;
> status = efi_relocate_kernel(sys_table, image_addr, *image_size,
>  *image_size,
> -dram_base + MAX_UNCOMP_KERNEL_SIZE, 0);
> +dram_base + MAX_UNCOMP_KERNEL_SIZE, 0, 
> 0);
> if (status != EFI_SUCCESS) {
> pr_efi_err(sys_table, "Failed to relocate kernel.\n");
> efi_free(sys_table, *reserve_size, *reserve_addr);
> diff --git 

Re: [PATCH] efi: make unexported efi_rci2_sysfs_init static

2019-09-25 Thread Ard Biesheuvel
On Wed, 25 Sep 2019 at 15:12, Ben Dooks  wrote:
>
> The efi_rci2_sysfs_init() is not used outside of rci2-table.c so
> make it static to silence the following sparse warning:
>
> drivers/firmware/efi/rci2-table.c:79:12: warning: symbol 
> 'efi_rci2_sysfs_init' was not declared. Should it be static?
>
> Signed-off-by: Ben Dooks 
> ---
>  drivers/firmware/efi/rci2-table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/rci2-table.c 
> b/drivers/firmware/efi/rci2-table.c
> index 3e290f96620a..76b0c354a027 100644
> --- a/drivers/firmware/efi/rci2-table.c
> +++ b/drivers/firmware/efi/rci2-table.c
> @@ -76,7 +76,7 @@ static u16 checksum(void)
> return chksum;
>  }
>
> -int __init efi_rci2_sysfs_init(void)
> +static int __init efi_rci2_sysfs_init(void)
>  {
> struct kobject *tables_kobj;
> int ret = -ENOMEM;
> --
> 2.23.0
>

Thanks Ben.

Queued in efi/urgent


Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

2019-09-25 Thread Jerry Snitselaar

On Wed Sep 25 19, Jarkko Sakkinen wrote:

On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:

On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
 wrote:
>
> From: Peter Jones 
>
> Some machines generate a lot of event log entries.  When we're
> iterating over them, the code removes the old mapping and adds a
> new one, so once we cross the page boundary we're unmapping the page
> with the count on it.  Hilarity ensues.
>
> This patch keeps the info from the header in local variables so we don't
> need to access that page again or keep track of if it's mapped.
>
> Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> Cc: linux-efi@vger.kernel.org
> Cc: linux-integr...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Signed-off-by: Peter Jones 
> Tested-by: Lyude Paul 
> Reviewed-by: Jarkko Sakkinen 
> Acked-by: Matthew Garrett 
> Acked-by: Ard Biesheuvel 
> Signed-off-by: Jarkko Sakkinen 

Thanks Jarkko.

Shall I take these through the EFI tree?


Would be great, if you could because I already sent one PR with fixes for
v5.4-rc1 yesterday.

/Jarkko


My patch collides with this, so I will submit a v3 that applies on top of
these once I've run a test with all 3 applied on this t480s.


Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

2019-09-25 Thread Jarkko Sakkinen
On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
> On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
>  wrote:
> >
> > From: Peter Jones 
> >
> > Some machines generate a lot of event log entries.  When we're
> > iterating over them, the code removes the old mapping and adds a
> > new one, so once we cross the page boundary we're unmapping the page
> > with the count on it.  Hilarity ensues.
> >
> > This patch keeps the info from the header in local variables so we don't
> > need to access that page again or keep track of if it's mapped.
> >
> > Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> > Cc: linux-efi@vger.kernel.org
> > Cc: linux-integr...@vger.kernel.org
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Peter Jones 
> > Tested-by: Lyude Paul 
> > Reviewed-by: Jarkko Sakkinen 
> > Acked-by: Matthew Garrett 
> > Acked-by: Ard Biesheuvel 
> > Signed-off-by: Jarkko Sakkinen 
> 
> Thanks Jarkko.
> 
> Shall I take these through the EFI tree?

Would be great, if you could because I already sent one PR with fixes for
v5.4-rc1 yesterday.

/Jarkko


[PATCH v2] tpm: only set efi_tpm_final_log_size after successful event log parsing

2019-09-25 Thread Jerry Snitselaar
If __calc_tpm2_event_size fails to parse an event it will return 0,
resulting tpm2_calc_event_log_size returning -1. Currently there is
no check of this return value, and efi_tpm_final_log_size can end up
being set to this negative value resulting in a panic like the
the one given below.

This has been reported with a number of systems now from different
vendors. In the case of the system I have access to the hash algorithm
id associated with an event isn't in the TCG registry, and a match
isn't found when walking the digest sizes array resulting in 0 being
returned by __calc_tpm2_event_size.  There are a number of other
points in __calc_tpm2_event_size where it could potentially return 0
and trigger the same outcome.

[0.774340] BUG: unable to handle page fault for address: bc8fc00866ad
[0.774788] #PF: supervisor read access in kernel mode
[0.774788] #PF: error_code(0x) - not-present page
[0.774788] PGD 107d36067 P4D 107d36067 PUD 107d37067 PMD 107d38067 PTE 0
[0.774788] Oops:  [#1] SMP PTI
[0.774788] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
5.3.0-0.rc2.1.elrdy.x86_64 #1
[0.774788] Hardware name: LENOVO 20HGS22D0W/20HGS22D0W, BIOS N1WET51W (1.30 
) 09/14/2018
[0.774788] RIP: 0010:memcpy_erms+0x6/0x10
[0.774788] Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 
83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1  a4 c3 
0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
[0.774788] RSP: :bc8fc0073b30 EFLAGS: 00010286
[0.774788] RAX: 9b1fc7c5b367 RBX: 9b1fc839 RCX: e962
[0.774788] RDX: e962 RSI: bc8fc00866ad RDI: 9b1fc7c5b367
[0.774788] RBP: 9b1c10ca7018 R08: bc8fc0085fff R09: 8063
[0.774788] R10: 1000 R11: 000fffe0 R12: 3367
[0.774788] R13: 9b1fcc47c010 R14: bc8fc0085000 R15: 0002
[0.774788] FS:  () GS:9b1fce20() 
knlGS:
[0.774788] CS:  0010 DS:  ES:  CR0: 80050033
[0.774788] CR2: bc8fc00866ad CR3: 00029f60a001 CR4: 003606f0
[0.774788] DR0:  DR1:  DR2: 
[0.774788] DR3:  DR6: fffe0ff0 DR7: 0400
[0.774788] Call Trace:
[0.774788]  tpm_read_log_efi+0x156/0x1a0
[0.774788]  tpm_bios_log_setup+0xc8/0x190
[0.774788]  tpm_chip_register+0x50/0x1c0
[0.774788]  tpm_tis_core_init.cold.9+0x28c/0x466
[0.774788]  tpm_tis_plat_probe+0xcc/0xea
[0.774788]  platform_drv_probe+0x35/0x80
[0.774788]  really_probe+0xef/0x390
[0.774788]  driver_probe_device+0xb4/0x100
[0.774788]  device_driver_attach+0x4f/0x60
[0.774788]  __driver_attach+0x86/0x140
[0.774788]  ? device_driver_attach+0x60/0x60
[0.774788]  bus_for_each_dev+0x76/0xc0
[0.774788]  ? klist_add_tail+0x3b/0x70
[0.774788]  bus_add_driver+0x14a/0x1e0
[0.774788]  ? tpm_init+0xea/0xea
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  driver_register+0x6b/0xb0
[0.774788]  ? tpm_init+0xea/0xea
[0.774788]  init_tis+0x86/0xd8
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  ? driver_register+0x94/0xb0
[0.774788]  do_one_initcall+0x46/0x1e4
[0.774788]  ? do_early_param+0x8e/0x8e
[0.774788]  kernel_init_freeable+0x199/0x242
[0.774788]  ? rest_init+0xaa/0xaa
[0.774788]  kernel_init+0xa/0x106
[0.774788]  ret_from_fork+0x35/0x40
[0.774788] Modules linked in:
[0.774788] CR2: bc8fc00866ad
[0.774788] ---[ end trace 42930799f8d6eaea ]---
[0.774788] RIP: 0010:memcpy_erms+0x6/0x10
[0.774788] Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 
83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1  a4 c3 
0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
[0.774788] RSP: :bc8fc0073b30 EFLAGS: 00010286
[0.774788] RAX: 9b1fc7c5b367 RBX: 9b1fc839 RCX: e962
[0.774788] RDX: e962 RSI: bc8fc00866ad RDI: 9b1fc7c5b367
[0.774788] RBP: 9b1c10ca7018 R08: bc8fc0085fff R09: 8063
[0.774788] R10: 1000 R11: 000fffe0 R12: 3367
[0.774788] R13: 9b1fcc47c010 R14: bc8fc0085000 R15: 0002
[0.774788] FS:  () GS:9b1fce20() 
knlGS:
[0.774788] CS:  0010 DS:  ES:  CR0: 80050033
[0.774788] CR2: bc8fc00866ad CR3: 00029f60a001 CR4: 003606f0
[0.774788] DR0:  DR1:  DR2: 
[0.774788] DR3:  DR6: fffe0ff0 DR7: 0400
[0.774788] Kernel panic - not syncing: Fatal exception
[0.774788] Kernel Offset: 0x1d00 from 0x8100 (relocation 
range: 0x8000-0xbfff)
[0.774788] ---[ end Kernel 

[PATCH] efi: make unexported efi_rci2_sysfs_init static

2019-09-25 Thread Ben Dooks
The efi_rci2_sysfs_init() is not used outside of rci2-table.c so
make it static to silence the following sparse warning:

drivers/firmware/efi/rci2-table.c:79:12: warning: symbol 'efi_rci2_sysfs_init' 
was not declared. Should it be static?

Signed-off-by: Ben Dooks 
---
 drivers/firmware/efi/rci2-table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/rci2-table.c 
b/drivers/firmware/efi/rci2-table.c
index 3e290f96620a..76b0c354a027 100644
--- a/drivers/firmware/efi/rci2-table.c
+++ b/drivers/firmware/efi/rci2-table.c
@@ -76,7 +76,7 @@ static u16 checksum(void)
return chksum;
 }
 
-int __init efi_rci2_sysfs_init(void)
+static int __init efi_rci2_sysfs_init(void)
 {
struct kobject *tables_kobj;
int ret = -ENOMEM;
-- 
2.23.0



Re: [RFC PATCH] tpm: only set efi_tpm_final_log_size after successful event log parsing

2019-09-25 Thread Jarkko Sakkinen
On Wed, Sep 18, 2019 at 12:16:26PM -0700, Jerry Snitselaar wrote:
> + if (tbl_size < 0) {
> + pr_err("Failed to parse event in TPM Final Event log\n");

FW_BUG?

> + goto calc_out;
> + }
> +
>   memblock_reserve((unsigned long)final_tbl,
>tbl_size + sizeof(*final_tbl));
> - early_memunmap(final_tbl, sizeof(*final_tbl));
>   efi_tpm_final_log_size = tbl_size;
>  
> +calc_out:
> + early_memunmap(final_tbl, sizeof(*final_tbl));

out_calc

>  out:
>   early_memunmap(log_tbl, sizeof(*log_tbl));
>   return ret;
> -- 
> 2.23.0
> 

/Jarkko


Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

2019-09-25 Thread Ard Biesheuvel
On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
 wrote:
>
> From: Peter Jones 
>
> Some machines generate a lot of event log entries.  When we're
> iterating over them, the code removes the old mapping and adds a
> new one, so once we cross the page boundary we're unmapping the page
> with the count on it.  Hilarity ensues.
>
> This patch keeps the info from the header in local variables so we don't
> need to access that page again or keep track of if it's mapped.
>
> Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> Cc: linux-efi@vger.kernel.org
> Cc: linux-integr...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Signed-off-by: Peter Jones 
> Tested-by: Lyude Paul 
> Reviewed-by: Jarkko Sakkinen 
> Acked-by: Matthew Garrett 
> Acked-by: Ard Biesheuvel 
> Signed-off-by: Jarkko Sakkinen 

Thanks Jarkko.

Shall I take these through the EFI tree?


> ---
>  include/linux/tpm_eventlog.h | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> index 63238c84dc0b..12584b69a3f3 100644
> --- a/include/linux/tpm_eventlog.h
> +++ b/include/linux/tpm_eventlog.h
> @@ -170,6 +170,7 @@ static inline int __calc_tpm2_event_size(struct 
> tcg_pcr_event2_head *event,
> u16 halg;
> int i;
> int j;
> +   u32 count, event_type;
>
> marker = event;
> marker_start = marker;
> @@ -190,16 +191,22 @@ static inline int __calc_tpm2_event_size(struct 
> tcg_pcr_event2_head *event,
> }
>
> event = (struct tcg_pcr_event2_head *)mapping;
> +   /*
> +* the loop below will unmap these fields if the log is larger than
> +* one page, so save them here for reference.
> +*/
> +   count = READ_ONCE(event->count);
> +   event_type = READ_ONCE(event->event_type);
>
> efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
>
> /* Check if event is malformed. */
> -   if (event->count > efispecid->num_algs) {
> +   if (count > efispecid->num_algs) {
> size = 0;
> goto out;
> }
>
> -   for (i = 0; i < event->count; i++) {
> +   for (i = 0; i < count; i++) {
> halg_size = sizeof(event->digests[i].alg_id);
>
> /* Map the digest's algorithm identifier */
> @@ -256,8 +263,9 @@ static inline int __calc_tpm2_event_size(struct 
> tcg_pcr_event2_head *event,
> + event_field->event_size;
> size = marker - marker_start;
>
> -   if ((event->event_type == 0) && (event_field->event_size == 0))
> +   if (event_type == 0 && event_field->event_size == 0)
> size = 0;
> +
>  out:
> if (do_mapping)
> TPM_MEMUNMAP(mapping, mapping_size);
> --
> 2.20.1
>


[PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

2019-09-25 Thread Jarkko Sakkinen
From: Peter Jones 

Some machines generate a lot of event log entries.  When we're
iterating over them, the code removes the old mapping and adds a
new one, so once we cross the page boundary we're unmapping the page
with the count on it.  Hilarity ensues.

This patch keeps the info from the header in local variables so we don't
need to access that page again or keep track of if it's mapped.

Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
Cc: linux-efi@vger.kernel.org
Cc: linux-integr...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Peter Jones 
Tested-by: Lyude Paul 
Reviewed-by: Jarkko Sakkinen 
Acked-by: Matthew Garrett 
Acked-by: Ard Biesheuvel 
Signed-off-by: Jarkko Sakkinen 
---
 include/linux/tpm_eventlog.h | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 63238c84dc0b..12584b69a3f3 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -170,6 +170,7 @@ static inline int __calc_tpm2_event_size(struct 
tcg_pcr_event2_head *event,
u16 halg;
int i;
int j;
+   u32 count, event_type;
 
marker = event;
marker_start = marker;
@@ -190,16 +191,22 @@ static inline int __calc_tpm2_event_size(struct 
tcg_pcr_event2_head *event,
}
 
event = (struct tcg_pcr_event2_head *)mapping;
+   /*
+* the loop below will unmap these fields if the log is larger than
+* one page, so save them here for reference.
+*/
+   count = READ_ONCE(event->count);
+   event_type = READ_ONCE(event->event_type);
 
efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
 
/* Check if event is malformed. */
-   if (event->count > efispecid->num_algs) {
+   if (count > efispecid->num_algs) {
size = 0;
goto out;
}
 
-   for (i = 0; i < event->count; i++) {
+   for (i = 0; i < count; i++) {
halg_size = sizeof(event->digests[i].alg_id);
 
/* Map the digest's algorithm identifier */
@@ -256,8 +263,9 @@ static inline int __calc_tpm2_event_size(struct 
tcg_pcr_event2_head *event,
+ event_field->event_size;
size = marker - marker_start;
 
-   if ((event->event_type == 0) && (event_field->event_size == 0))
+   if (event_type == 0 && event_field->event_size == 0)
size = 0;
+
 out:
if (do_mapping)
TPM_MEMUNMAP(mapping, mapping_size);
-- 
2.20.1



[PATCH v2 2/2] efi+tpm: don't traverse an event log with no events

2019-09-25 Thread Jarkko Sakkinen
From: Peter Jones 

When there are no entries to put into the final event log, some machines
will return the template they would have populated anyway.  In this case
the nr_events field is 0, but the rest of the log is just garbage.

This patch stops us from trying to iterate the table with
__calc_tpm2_event_size() when the number of events in the table is 0.

Fixes: c46f3405692d ("tpm: Reserve the TPM final events table")
Cc: linux-efi@vger.kernel.org
Cc: linux-integr...@vger.kernel.org
Cc: sta...@vger.kernel.org
Signed-off-by: Peter Jones 
Tested-by: Lyude Paul 
Reviewed-by: Jarkko Sakkinen 
Acked-by: Matthew Garrett 
Acked-by: Ard Biesheuvel 
Signed-off-by: Jarkko Sakkinen 
---
 drivers/firmware/efi/tpm.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 1d3f5ca3eaaf..b9ae5c6f9b9c 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -75,11 +75,16 @@ int __init efi_tpm_eventlog_init(void)
goto out;
}
 
-   tbl_size = tpm2_calc_event_log_size((void *)efi.tpm_final_log
-   + sizeof(final_tbl->version)
-   + sizeof(final_tbl->nr_events),
-   final_tbl->nr_events,
-   log_tbl->log);
+   tbl_size = 0;
+   if (final_tbl->nr_events != 0) {
+   void *events = (void *)efi.tpm_final_log
+   + sizeof(final_tbl->version)
+   + sizeof(final_tbl->nr_events);
+
+   tbl_size = tpm2_calc_event_log_size(events,
+   final_tbl->nr_events,
+   log_tbl->log);
+   }
memblock_reserve((unsigned long)final_tbl,
 tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
-- 
2.20.1



Re: [PATCH v2] x86, efi: never relocate kernel below lowest acceptable address

2019-09-25 Thread Jarkko Sakkinen
On Fri, Sep 20, 2019 at 12:05:21AM +0800, Kairui Song wrote:
> Currently, kernel fails to boot on some HyperV VMs when using EFI.
> And it's a potential issue on all platforms.
> 
> It's caused a broken kernel relocation on EFI systems, when below three
> conditions are met:
> 
> 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
>by the loader.
> 2. There isn't enough room to contain the kernel, starting from the
>default load address (eg. something else occupied part the region).
> 3. In the memmap provided by EFI firmware, there is a memory region
>starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
>kernel.
> 
> Efi stub will perform a kernel relocation when condition 1 is met. But
> due to condition 2, efi stub can't relocate kernel to the preferred
> address, so it fallback to query and alloc from EFI firmware for lowest
> usable memory region.
> 
> It's incorrect to use the lowest memory address. In later stage, kernel
> will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> but efi stub will end up relocating kernel below it.
> 
> Then before the kernel decompressing. Kernel will do another relocation
> to address not lower than LOAD_PHYSICAL_ADDR, this time the relocate will
> over write the blockage at the default load address, which efi stub tried
> to avoid, and lead to unexpected behavior. Beside, the memory region it
> writes to is not allocated from EFI firmware, which is also wrong.
> 
> To fix it, just don't let efi stub relocate the kernel to any address
> lower than lowest acceptable address.
> 
> Signed-off-by: Kairui Song 

Acked-by:  

/Jarkko


[PATCH 2/2] x86/efi: Fix memory leak for EFI memmap reservations

2019-09-25 Thread Yunfeng Ye
There are some memory leaks in failure path after efi_memmap_alloc().
add efi_memmap_free() for this situation.

Fixes: 816e76129ed5 ("efi: Allow drivers to reserve boot services forever")
Signed-off-by: Yunfeng Ye 
---
 arch/x86/platform/efi/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 3b9fd67..a755f35 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -285,6 +285,7 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)

new = early_memremap(new_phys, new_size);
if (!new) {
+   efi_memmap_free(new_phys, num_entries);
pr_err("Failed to map new boot services memmap\n");
return;
}
@@ -470,6 +471,7 @@ void __init efi_free_boot_services(void)

new = memremap(new_phys, new_size, MEMREMAP_WB);
if (!new) {
+   efi_memmap_free(new_phys, num_entries);
pr_err("Failed to map new EFI memmap\n");
return;
}
@@ -493,6 +495,7 @@ void __init efi_free_boot_services(void)
memunmap(new);

if (efi_memmap_install(new_phys, num_entries)) {
+   efi_memmap_free(new_phys, num_entries);
pr_err("Could not install new EFI memmap\n");
return;
}
-- 
1.8.3.1



[PATCH 1/2] efi: Add efi_memmap_free() to free EFI memory map

2019-09-25 Thread Yunfeng Ye
In efi_fake_memmap(), the commit 20b1e22d01a4 ("x86/efi: Don't allocate
memmap through memblock after mm_init()") replace memblock_alloc() with
efi_memmap_alloc(), but there is no matching modification of
memblock_free() when early_memremap() fail.

Fix this by adding efi_memmap_free() to instead of memblock_free().

Fixes: 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after 
mm_init()")
Signed-off-by: Yunfeng Ye 
---
 drivers/firmware/efi/fake_mem.c |  2 +-
 drivers/firmware/efi/memmap.c   | 34 ++
 include/linux/efi.h |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c
index 9501edc..c2f69f6 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/drivers/firmware/efi/fake_mem.c
@@ -65,7 +65,7 @@ void __init efi_fake_memmap(void)
new_memmap = early_memremap(new_memmap_phy,
efi.memmap.desc_size * new_nr_map);
if (!new_memmap) {
-   memblock_free(new_memmap_phy, efi.memmap.desc_size * 
new_nr_map);
+   efi_memmap_free(new_memmap_phy, new_nr_map);
return;
}

diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 38b686c..35dc189 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -18,6 +18,11 @@ static phys_addr_t __init __efi_memmap_alloc_early(unsigned 
long size)
return memblock_phys_alloc(size, SMP_CACHE_BYTES);
 }

+static void __init __efi_memmap_free_early(phys_addr_t addr, unsigned long 
size)
+{
+   memblock_free(addr, size);
+}
+
 static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
 {
unsigned int order = get_order(size);
@@ -29,6 +34,15 @@ static phys_addr_t __init __efi_memmap_alloc_late(unsigned 
long size)
return PFN_PHYS(page_to_pfn(p));
 }

+static void __init __efi_memmap_free_late(phys_addr_t addr, unsigned long size)
+{
+   unsigned int order = get_order(size);
+   struct page *p = pfn_to_page(PHYS_PFN(addr));
+
+   if (p)
+   __free_pages(p, order);
+}
+
 /**
  * efi_memmap_alloc - Allocate memory for the EFI memory map
  * @num_entries: Number of entries in the allocated map.
@@ -50,6 +64,26 @@ phys_addr_t __init efi_memmap_alloc(unsigned int num_entries)
 }

 /**
+ * efi_memmap_free - Free memory for the EFI memory map
+ * @addr: Physical address of the EFI memory map to be freed.
+ * @num_entries: Number of the EFI memory map entries.
+ *
+ * Depending on whether mm_init() has already been invoked or not,
+ * either memblock or "normal" page free is used.
+ */
+void __init efi_memmap_free(phys_addr_t addr, unsigned int num_entries)
+{
+   unsigned long size = num_entries * efi.memmap.desc_size;
+
+   if (slab_is_available()) {
+   __efi_memmap_free_late(addr, size);
+
+   return;
+   }
+   __efi_memmap_free_early(addr, size);
+}
+
+/**
  * __efi_memmap_init - Common code for mapping the EFI memory map
  * @data: EFI memory map data
  * @late: Use early or late mapping function?
diff --git a/include/linux/efi.h b/include/linux/efi.h
index bd38370..8bb741a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1057,6 +1057,7 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes,
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);

 extern phys_addr_t __init efi_memmap_alloc(unsigned int num_entries);
+extern void __init efi_memmap_free(phys_addr_t addr, unsigned int num_entries);
 extern int __init efi_memmap_init_early(struct efi_memory_map_data *data);
 extern int __init efi_memmap_init_late(phys_addr_t addr, unsigned long size);
 extern void __init efi_memmap_unmap(void);
-- 
1.8.3.1