linux-kernel - Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for kexec kernel

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: <CAO7dBbVXD+3tq_iQMKtX+Vw7csPfzPJdOWVduMENyO6i7p0b0w@mail.gmail.com>
Date:   Thu, 1 Jun 2023 16:25:31 +0800
From:   Tao Liu <ltao@...hat.com>
To:     Baoquan He <bhe@...hat.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        ardb@...nel.org, linux-kernel@...r.kernel.org, dyoung@...hat.com,
        kexec@...ts.infradead.org, linux-efi@...r.kernel.org
Subject: Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for
 kexec kernel

Hi Baoquan,

On Thu, Jun 1, 2023 at 4:13 PM Baoquan He <bhe@...hat.com> wrote:
>
> On 06/01/23 at 03:20pm, Tao Liu wrote:
> > A kexec kernel bootup hang is observed on Intel Atom cpu due to unmapped
> > EFI config table.
> >
> > Currently EFI system table is identity-mapped for the kexec kernel, but EFI
> > config table is not mapped explicitly:
> >
> >     commit 6bbeb276b71f ("x86/kexec: Add the EFI system tables and ACPI
> >                           tables to the ident map")
> >
> > Later in the following 2 commits, EFI config table will be accessed when
> > enabling sev at kernel startup. This may result in a page fault due to EFI
> > config table's unmapped address. Since the page fault occurs at an early
> > stage, it is unrecoverable and kernel hangs.
> >
> >     commit ec1c66af3a30 ("x86/compressed/64: Detect/setup SEV/SME features
> >                           earlier during boot")
> >     commit c01fce9cef84 ("x86/compressed: Add SEV-SNP feature
> >                           detection/setup")
> >
> > In addition, the issue doesn't appear on all systems, because the kexec
> > kernel uses Page Size Extension (PSE) for identity mapping. In most cases,
> > EFI config table can end up to be mapped into due to 1 GB page size.
> > However if nogbpages is set, or cpu doesn't support pdpe1gb feature
> > (e.g Intel Atom x6425RE cpu), EFI config table may not be mapped into
> > due to 2 MB page size, thus a page fault hang is more likely to happen.
> >
> > This patch will make sure the EFI config table is always mapped.
> >
> > Signed-off-by: Tao Liu <ltao@...hat.com>
> > ---
> > Changes in v2:
> > - Rephrase the change log based on Baoquan's suggestion.
> > - Rename map_efi_sys_cfg_tab() to map_efi_tables().
> > - Link to v1: https://lore.kernel.org/kexec/20230525094914.23420-1-ltao@redhat.com/
> > ---
> >  arch/x86/kernel/machine_kexec_64.c | 35 ++++++++++++++++++++++++++----
> >  1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> > index 1a3e2c05a8a5..664aefa6e896 100644
> > --- a/arch/x86/kernel/machine_kexec_64.c
> > +++ b/arch/x86/kernel/machine_kexec_64.c
> > @@ -28,6 +28,7 @@
> >  #include <asm/setup.h>
> >  #include <asm/set_memory.h>
> >  #include <asm/cpu.h>
> > +#include <asm/efi.h>
> >
> >  #ifdef CONFIG_ACPI
> >  /*
> > @@ -86,10 +87,12 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
> >  #endif
> >
> >  static int
> > -map_efi_systab(struct x86_mapping_info *info, pgd_t *level4p)
> > +map_efi_tables(struct x86_mapping_info *info, pgd_t *level4p)
> >  {
> >  #ifdef CONFIG_EFI
> >       unsigned long mstart, mend;
> > +     void *kaddr;
> > +     int ret;
> >
> >       if (!efi_enabled(EFI_BOOT))
> >               return 0;
> > @@ -105,6 +108,30 @@ map_efi_systab(struct x86_mapping_info *info, pgd_t *level4p)
> >       if (!mstart)
> >               return 0;
> >
> > +     ret = kernel_ident_mapping_init(info, level4p, mstart, mend);
> > +     if (ret)
> > +             return ret;
> > +
> > +     kaddr = memremap(mstart, mend - mstart, MEMREMAP_WB);
> > +     if (!kaddr) {
> > +             pr_err("Could not map UEFI system table\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     mstart = efi_config_table;
> > +
> > +     if (efi_enabled(EFI_64BIT)) {
> > +             efi_system_table_64_t *stbl = (efi_system_table_64_t *)kaddr;
> > +
> > +             mend = mstart + sizeof(efi_config_table_64_t) * stbl->nr_tables;
> > +     } else {
> > +             efi_system_table_32_t *stbl = (efi_system_table_32_t *)kaddr;
> > +
> > +             mend = mstart + sizeof(efi_config_table_32_t) * stbl->nr_tables;
> > +     }
> > +
> > +     memunmap(kaddr);
> > +
> >       return kernel_ident_mapping_init(info, level4p, mstart, mend);
> >  #endif
> >       return 0;
> > @@ -244,10 +271,10 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> >       }
> >
> >       /*
> > -      * Prepare EFI systab and ACPI tables for kexec kernel since they are
> > -      * not covered by pfn_mapped.
> > +      * Prepare EFI systab, config table and ACPI tables for kexec kernel
>
> The code comment need be updated too?
>
>          * Prepare EFI tables and ACPI tables for kexec kernel since they are
>          * not covered by pfn_mapped.
>
> Other than this nit, this patch looks good to me, thanks.
>

Thanks for the patch review! I'm OK with the comment update, but I
prefer to leave it as it is. Since the comment provides more details:
there are systab and config tables mapped instead of all efi tables.

Thanks,
Tao Liu

> Acked-by: Baoquan He <bhe@...hat.com>
>
>
> > +      * since they are not covered by pfn_mapped.
> >        */
> > -     result = map_efi_systab(&info, level4p);
> > +     result = map_efi_tables(&info, level4p);
> >       if (result)
> >               return result;
> >
> > --
> > 2.33.1
> >
>