linux-kernel - Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKv+Gu-cdhvAM2Vkqv8uokZAqD3k0kLbGu1UYwgQOYj5Es0g=g@mail.gmail.com>
Date:   Thu, 12 Jan 2017 16:15:59 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Dave Young <dyoung@...hat.com>
Cc:     Matt Fleming <matt@...eblueprint.co.uk>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Nicolai Stange <nicstange@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        bhsharma@...hat.com
Subject: Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into
 boot mem areas

Hello Dave,

On 12 January 2017 at 09:41, Dave Young <dyoung@...hat.com> wrote:
> There are memory ranges like below when I testing early efi_mem_reserve:
>
> efi: mem62: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem63: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem64: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem65: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem66: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem67: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>

Did you spot Peter's patch to prune invalid memmap entries?

https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229

I would expect this patch to no longer be necessary with that in place, no?


> So efi_memmap_insert will run into inserting same region multiple times,
> also because efi_memmap_insert does not consider the duplicate ranges it will
> cause memmap buffer overflow due to the size is pre-calculated, and kernel boot
> fail with a panic.  We did not detect such issue because current users of
> efi_mem_insert do it very late after switching to virtual mode, at that time
> the new cooked efi.memmap contains only runtime needed memory ranges.
>
> efi_mem_reserve cares only about boot services regions and maybe loader areas.
> So add a new argument to efi_memmap_insert for this purpose.
>
> Later patches depend on this one for moving bgrt reservation to early code.
>
> Signed-off-by: Dave Young <dyoung@...hat.com>
> ---
>  arch/x86/platform/efi/quirks.c  |    2 +-
>  drivers/firmware/efi/fake_mem.c |    3 ++-
>  drivers/firmware/efi/memmap.c   |    8 +++++++-
>  include/linux/efi.h             |    4 ++--
>  4 files changed, 12 insertions(+), 5 deletions(-)
>
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
>   * to see how large @buf needs to be.
>   */
>  void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> -                             struct efi_mem_range *mem)
> +                             struct efi_mem_range *mem, bool boot_only)
>  {
>         u64 m_start, m_end, m_attr;
>         efi_memory_desc_t *md;
> @@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
>                 start = md->phys_addr;
>                 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
>
> +               if (boot_only && !(md->type == EFI_LOADER_DATA ||
> +                   md->type == EFI_LOADER_CODE ||
> +                   md->type == EFI_BOOT_SERVICES_CODE ||
> +                   md->type == EFI_BOOT_SERVICES_DATA))
> +                       continue;
> +
>                 if (m_start <= start && end <= m_end)
>                         md->attribute |= m_attr;
>
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -226,7 +226,7 @@ void __init efi_arch_mem_reserve(phys_ad
>                 return;
>         }
>
> -       efi_memmap_insert(&efi.memmap, new, &mr);
> +       efi_memmap_insert(&efi.memmap, new, &mr, true);
>         early_memunmap(new, new_size);
>
>         efi_memmap_install(new_phys, num_entries);
> --- linux-x86.orig/drivers/firmware/efi/fake_mem.c
> +++ linux-x86/drivers/firmware/efi/fake_mem.c
> @@ -85,7 +85,8 @@ void __init efi_fake_memmap(void)
>         }
>
>         for (i = 0; i < nr_fake_mem; i++)
> -               efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i]);
> +               efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i],
> +                                 false);
>
>         /* swap into new EFI memmap */
>         early_memunmap(new_memmap, efi.memmap.desc_size * new_nr_map);
> --- linux-x86.orig/include/linux/efi.h
> +++ linux-x86/include/linux/efi.h
> @@ -957,8 +957,8 @@ extern int __init efi_memmap_install(phy
>  extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
>                                          struct range *range);
>  extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
> -                                    void *buf, struct efi_mem_range *mem);
> -
> +                                    void *buf, struct efi_mem_range *mem,
> +                                    bool boot_only);
>  extern int efi_config_init(efi_config_table_type_t *arch_tables);
>  #ifdef CONFIG_EFI_ESRT
>  extern void __init efi_esrt_init(void);
>
>