linux-kernel - Re: [PATCH] efi: discover ESRT table on Xen PV too

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: <CAMj1kXEQ2mpmcNke0K2MZPAAo9wGZ4h3pCmMg9Hm7CPXOCV7fQ@mail.gmail.com>
Date:   Mon, 17 Aug 2020 10:16:07 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Marek Marczykowski-Górecki 
        <marmarek@...isiblethingslab.com>
Cc:     linux-efi <linux-efi@...r.kernel.org>, norbert.kaminski@...eb.com,
        xen-devel@...ts.xenproject.org,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] efi: discover ESRT table on Xen PV too

Hi Marek,

On Sun, 16 Aug 2020 at 02:20, Marek Marczykowski-Górecki
<marmarek@...isiblethingslab.com> wrote:
>
> In case of Xen PV dom0, Xen passes along info about system tables (see
> arch/x86/xen/efi.c), but not the memory map from EFI. This makes sense
> as it is Xen responsible for managing physical memory address space.
> In this case, it doesn't make sense to condition using ESRT table on
> availability of EFI memory map, as it isn't Linux kernel responsible for
> it. Skip this part on Xen PV (let Xen do the right thing if it deems
> necessary) and use ESRT table normally.
>
> This is a requirement for using fwupd in PV dom0 to update UEFI using
> capsules.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> ---
>  drivers/firmware/efi/esrt.c | 47 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index d5915272141f..5c49f2aaa4b1 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -245,36 +245,38 @@ void __init efi_esrt_init(void)
>         int rc;
>         phys_addr_t end;
>
> -       if (!efi_enabled(EFI_MEMMAP))
> +       if (!efi_enabled(EFI_MEMMAP) && !efi_enabled(EFI_PARAVIRT))
>                 return;
>
>         pr_debug("esrt-init: loading.\n");
>         if (!esrt_table_exists())
>                 return;
>
> -       rc = efi_mem_desc_lookup(efi.esrt, &md);
> -       if (rc < 0 ||
> -           (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> -            md.type != EFI_BOOT_SERVICES_DATA &&
> -            md.type != EFI_RUNTIME_SERVICES_DATA)) {
> -               pr_warn("ESRT header is not in the memory map.\n");
> -               return;
> -       }
> +       if (efi_enabled(EFI_MEMMAP)) {
> +               rc = efi_mem_desc_lookup(efi.esrt, &md);
> +               if (rc < 0 ||
> +                   (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> +                    md.type != EFI_BOOT_SERVICES_DATA &&
> +                    md.type != EFI_RUNTIME_SERVICES_DATA)) {
> +                       pr_warn("ESRT header is not in the memory map.\n");
> +                       return;
> +               }
>
> -       max = efi_mem_desc_end(&md);
> -       if (max < efi.esrt) {
> -               pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> -                      (void *)efi.esrt, (void *)max);
> -               return;
> -       }
> +               max = efi_mem_desc_end(&md);
> +               if (max < efi.esrt) {
> +                       pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> +                              (void *)efi.esrt, (void *)max);
> +                       return;
> +               }
>
> -       size = sizeof(*esrt);
> -       max -= efi.esrt;
> +               size = sizeof(*esrt);
> +               max -= efi.esrt;
>
> -       if (max < size) {
> -               pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
> -                      size, max);
> -               return;
> +               if (max < size) {
> +                       pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
> +                              size, max);
> +                       return;
> +               }
>         }
>
>         va = early_memremap(efi.esrt, size);
> @@ -331,7 +333,8 @@ void __init efi_esrt_init(void)
>
>         end = esrt_data + size;
>         pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data, &end);
> -       if (md.type == EFI_BOOT_SERVICES_DATA)
> +
> +       if (efi_enabled(EFI_MEMMAP) && md.type == EFI_BOOT_SERVICES_DATA)
>                 efi_mem_reserve(esrt_data, esrt_data_size);
>

This does not look correct to me. Why doesn't the region need to be
reserved on a Xen boot? The OS may overwrite it otherwise.


>         pr_debug("esrt-init: loaded.\n");
> --
> 2.25.4
>