Re: [PATCH] OvmfPkg/OvmfXen: Fix S3

[Ard Biesheuvel]

On Thu, 13 Jul 2023 at 12:48, Xenia Ragiadakou  wrote:
>
> Currently, resuming an S3 suspended guest results in the following
> assertion failure:
> ASSERT 
> MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.c(41): 
> MemoryLength > 0
> This happens because some parts of the S3 suspend and resume paths
> are missing in order for S3 to work. For instance, the variables
> mS3AcpiReservedMemoryBase and mS3AcpiReservedMemoryBase are not
> initialized, regions that are used on S3 resume are either missing
> or not marked as ACPI NVS memory and can be corrupted by the OS.
> This patch adds the missing parts based heavily on the existing S3
> implementation of other virtual platforms.
>
> For S3 support, the provision of fw_cfg is required in order for
> suspend states to be retrieved.
>
> Another issue noticed is that when CalibrateLapicTimer() is called
> on S3 resume path, the shared info page is remapped to a different
> guest physical address. This remapping happens under guest's feet,
> so any subsequent attempt of the guest to access the shared info
> page results in nested page faults. This patch removes any local
> APIC timer initializion and calibration from S3 resume path.
>
> Signed-off-by: Xenia Ragiadakou 

Anyone care to review this?

> ---
>  OvmfPkg/XenPlatformPei/Fv.c   |  2 +-
>  OvmfPkg/XenPlatformPei/MemDetect.c| 60 ++-
>  OvmfPkg/XenPlatformPei/Platform.c | 11 -
>  OvmfPkg/XenPlatformPei/Platform.h |  2 +
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  7 +++
>  5 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/XenPlatformPei/Fv.c b/OvmfPkg/XenPlatformPei/Fv.c
> index 871a2c1c5b..37ecb3cfee 100644
> --- a/OvmfPkg/XenPlatformPei/Fv.c
> +++ b/OvmfPkg/XenPlatformPei/Fv.c
> @@ -37,7 +37,7 @@ PeiFvInitialization (
>BuildMemoryAllocationHob (
>  PcdGet32 (PcdOvmfPeiMemFvBase),
>  PcdGet32 (PcdOvmfPeiMemFvSize),
> -EfiBootServicesData
> +mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
>  );
>
>//
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c 
> b/OvmfPkg/XenPlatformPei/MemDetect.c
> index e552e7a55e..1724a4988f 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -283,6 +283,19 @@ PublishPeiMemory (
>
>LowerMemorySize = GetSystemMemorySizeBelow4gb ();
>
> +  //
> +  // If S3 is supported, then the S3 permanent PEI memory is placed next,
> +  // downwards. Its size is primarily dictated by CpuMpPei. The formula below
> +  // is an approximation.
> +  //
> +  if (mS3Supported) {
> +mS3AcpiReservedMemorySize = SIZE_512KB +
> +PcdGet32 (PcdCpuMaxLogicalProcessorNumber) *
> +PcdGet32 (PcdCpuApStackSize);
> +mS3AcpiReservedMemoryBase = LowerMemorySize - mS3AcpiReservedMemorySize;
> +LowerMemorySize   = mS3AcpiReservedMemoryBase;
> +  }
> +
>if (mBootMode == BOOT_ON_S3_RESUME) {
>  MemoryBase = mS3AcpiReservedMemoryBase;
>  MemorySize = mS3AcpiReservedMemorySize;
> @@ -328,6 +341,51 @@ InitializeRamRegions (
>  {
>XenPublishRamRegions ();
>
> +  if (mS3Supported && (mBootMode != BOOT_ON_S3_RESUME)) {
> +//
> +// This is the memory range that will be used for PEI on S3 resume
> +//
> +BuildMemoryAllocationHob (
> +  mS3AcpiReservedMemoryBase,
> +  mS3AcpiReservedMemorySize,
> +  EfiACPIMemoryNVS
> +  );
> +
> +//
> +// Cover the initial RAM area used as stack and temporary PEI heap.
> +//
> +// This is reserved as ACPI NVS so it can be used on S3 resume.
> +//
> +BuildMemoryAllocationHob (
> +  PcdGet32 (PcdOvmfSecPeiTempRamBase),
> +  PcdGet32 (PcdOvmfSecPeiTempRamSize),
> +  EfiACPIMemoryNVS
> +  );
> +
> +//
> +// SEC stores its table of GUIDed section handlers here.
> +//
> +BuildMemoryAllocationHob (
> +  PcdGet64 (PcdGuidedExtractHandlerTableAddress),
> +  PcdGet32 (PcdGuidedExtractHandlerTableSize),
> +  EfiACPIMemoryNVS
> +  );
> +
> + #ifdef MDE_CPU_X64
> +//
> +// Reserve the initial page tables built by the reset vector code.
> +//
> +// Since this memory range will be used by the Reset Vector on S3
> +// resume, it must be reserved as ACPI NVS.
> +//
> +BuildMemoryAllocationHob (
> +  (EFI_PHYSICAL_ADDRESS)(UINTN)PcdGet32 (PcdOvmfSecPageTablesBase),
> +  (UINT64)(UINTN)PcdGet32 (PcdOvmfSecPageTablesSize),
> +  EfiACPIMemoryNVS
> +  );
> + #endif
> +  }
> +
>if (mBootMode != BOOT_ON_S3_RESUME) {
>  //
>  // Reserve the lock box storage area
> @@ -346,7 +404,7 @@ InitializeRamRegions (
>  BuildMemoryAllocationHob (
>(EFI_PHYSICAL_ADDRESS)(UINTN)PcdGet32 (PcdOvmfLockBoxStorageBase),
>(UINT64)(UINTN)PcdGet32 (PcdOvmfLockBoxStorageSize),
> -  EfiBootServicesData
> +  mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
>

Re: [PATCH v3 0/5] efi: Support ESRT under Xen

[Ard Biesheuvel]

On Thu, 19 Jan 2023 at 20:04, Demi Marie Obenour
 wrote:
>
> This patch series fixes handling of EFI tables when running under Xen.
> These fixes allow the ESRT to be loaded when running paravirtualized in
> dom0, making the use of EFI capsule updates possible.
>
> Demi Marie Obenour (5):
>   efi: memmap: Disregard bogus entries instead of returning them
>   efi: xen: Implement memory descriptor lookup based on hypercall
>   efi: Apply allowlist to EFI configuration tables when running under
> Xen
>   efi: Actually enable the ESRT under Xen
>   efi: Warn if trying to reserve memory under Xen
>

I have given these a spin on a system with a dodgy ESRT (the region in
question is not covered by the memory map at all), and things are
exactly as broken before, which is good.

I have queued these up in efi/next now, they should appear in -next tomorrow.


>  drivers/firmware/efi/efi.c  | 22 -
>  drivers/firmware/efi/esrt.c | 15 +++--
>  drivers/xen/efi.c   | 61 +
>  include/linux/efi.h |  3 ++
>  4 files changed, 90 insertions(+), 11 deletions(-)
>
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Thu, 6 Oct 2022 at 19:22, Demi Marie Obenour
 wrote:
>
> On Thu, Oct 06, 2022 at 06:19:35PM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Oct 2022 at 16:43, Demi Marie Obenour
> >  wrote:
> > >
> > > On Thu, Oct 06, 2022 at 09:31:47AM +0200, Ard Biesheuvel wrote:
> > > > On Thu, 6 Oct 2022 at 03:41, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > On Wed, Oct 05, 2022 at 11:28:29PM +0200, Ard Biesheuvel wrote:
> > > > > > On Wed, 5 Oct 2022 at 20:11, Demi Marie Obenour
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Wed, Oct 05, 2022 at 08:15:07AM +0200, Jan Beulich wrote:
> > > > > > > > On 04.10.2022 17:46, Demi Marie Obenour wrote:
> > > > > > > > > Linux has a function called efi_mem_reserve() that is used to 
> > > > > > > > > reserve
> > > > > > > > > EfiBootServicesData memory that contains e.g. EFI 
> > > > > > > > > configuration tables.
> > > > > > > > > This function does not work under Xen because Xen could have 
> > > > > > > > > already
> > > > > > > > > clobbered the memory.  efi_mem_reserve() not working is the 
> > > > > > > > > whole reason
> > > > > > > > > for this thread, as it prevents EFI tables that are in
> > > > > > > > > EfiBootServicesData from being used under Xen.
> > > > > > > > >
> > > > > > > > > A much nicer approach would be for Xen to reserve boot 
> > > > > > > > > services memory
> > > > > > > > > unconditionally, but provide a hypercall that dom0 could used 
> > > > > > > > > to free
> > > > > > > > > the parts of EfiBootServicesData memory that are no longer 
> > > > > > > > > needed.  This
> > > > > > > > > would allow efi_mem_reserve() to work normally.
> > > > > > > >
> > > > > > > > efi_mem_reserve() actually working would be a layering 
> > > > > > > > violation;
> > > > > > > > controlling the EFI memory map is entirely Xen's job.
> > > > > > >
> > > > > > > Doing this properly would require Xen to understand all of the EFI
> > > > > > > tables that could validly be in EfiBootServices* and which could 
> > > > > > > be of
> > > > > > > interest to dom0.  It might (at least on some very buggy firmware)
> > > > > > > require a partial ACPI and/or SMBIOS implementation too, if the 
> > > > > > > firmware
> > > > > > > decided to put an ACPI or SMBIOS table in EfiBootServices*.
> > > > > > >
> > > > > > > > As to the hypercall you suggest - I wouldn't mind its addition, 
> > > > > > > > but only
> > > > > > > > for the case when -mapbs is used. As I've indicated before, I'm 
> > > > > > > > of the
> > > > > > > > opinion that default behavior should be matching the intentions 
> > > > > > > > of the
> > > > > > > > spec, and the intention of EfiBootServices* is for the space to 
> > > > > > > > be
> > > > > > > > reclaimed. Plus I'm sure you realize there's a caveat with Dom0 
> > > > > > > > using
> > > > > > > > that hypercall: It might use it for regions where data lives 
> > > > > > > > which it
> > > > > > > > wouldn't care about itself, but which an eventual kexec-ed (or 
> > > > > > > > alike)
> > > > > > > > entity would later want to consume. Code/data potentially 
> > > > > > > > usable by
> > > > > > > > _anyone_ between two resets of the system cannot legitimately 
> > > > > > > > be freed
> > > > > > > > (and hence imo is wrong to live in EfiBootServices* regions).
> > > > > > >
> > > > > > > I agree, but currently some such data *is* in EfiBootServices* 
> > > > > > > regions,
> > > > > > > sadly.  When -mapbs is *not* used, I recommend uninstalling all 
> > >

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Thu, 6 Oct 2022 at 16:43, Demi Marie Obenour
 wrote:
>
> On Thu, Oct 06, 2022 at 09:31:47AM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Oct 2022 at 03:41, Demi Marie Obenour
> >  wrote:
> > >
> > > On Wed, Oct 05, 2022 at 11:28:29PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 5 Oct 2022 at 20:11, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > On Wed, Oct 05, 2022 at 08:15:07AM +0200, Jan Beulich wrote:
> > > > > > On 04.10.2022 17:46, Demi Marie Obenour wrote:
> > > > > > > Linux has a function called efi_mem_reserve() that is used to 
> > > > > > > reserve
> > > > > > > EfiBootServicesData memory that contains e.g. EFI configuration 
> > > > > > > tables.
> > > > > > > This function does not work under Xen because Xen could have 
> > > > > > > already
> > > > > > > clobbered the memory.  efi_mem_reserve() not working is the whole 
> > > > > > > reason
> > > > > > > for this thread, as it prevents EFI tables that are in
> > > > > > > EfiBootServicesData from being used under Xen.
> > > > > > >
> > > > > > > A much nicer approach would be for Xen to reserve boot services 
> > > > > > > memory
> > > > > > > unconditionally, but provide a hypercall that dom0 could used to 
> > > > > > > free
> > > > > > > the parts of EfiBootServicesData memory that are no longer 
> > > > > > > needed.  This
> > > > > > > would allow efi_mem_reserve() to work normally.
> > > > > >
> > > > > > efi_mem_reserve() actually working would be a layering violation;
> > > > > > controlling the EFI memory map is entirely Xen's job.
> > > > >
> > > > > Doing this properly would require Xen to understand all of the EFI
> > > > > tables that could validly be in EfiBootServices* and which could be of
> > > > > interest to dom0.  It might (at least on some very buggy firmware)
> > > > > require a partial ACPI and/or SMBIOS implementation too, if the 
> > > > > firmware
> > > > > decided to put an ACPI or SMBIOS table in EfiBootServices*.
> > > > >
> > > > > > As to the hypercall you suggest - I wouldn't mind its addition, but 
> > > > > > only
> > > > > > for the case when -mapbs is used. As I've indicated before, I'm of 
> > > > > > the
> > > > > > opinion that default behavior should be matching the intentions of 
> > > > > > the
> > > > > > spec, and the intention of EfiBootServices* is for the space to be
> > > > > > reclaimed. Plus I'm sure you realize there's a caveat with Dom0 
> > > > > > using
> > > > > > that hypercall: It might use it for regions where data lives which 
> > > > > > it
> > > > > > wouldn't care about itself, but which an eventual kexec-ed (or 
> > > > > > alike)
> > > > > > entity would later want to consume. Code/data potentially usable by
> > > > > > _anyone_ between two resets of the system cannot legitimately be 
> > > > > > freed
> > > > > > (and hence imo is wrong to live in EfiBootServices* regions).
> > > > >
> > > > > I agree, but currently some such data *is* in EfiBootServices* 
> > > > > regions,
> > > > > sadly.  When -mapbs is *not* used, I recommend uninstalling all of the
> > > > > configuration tables that point to EfiBootServicesData memory before
> > > > > freeing that memory.
> > > > >
> > > >
> > > > That seems like a reasonable approach to me. Tables like MEMATTR or
> > > > RT_PROP are mostly relevant for bare metal where the host kernel maps
> > > > the runtime services, and in general, passing on these tables without
> > > > knowing what they do is kind of fishy anyway. You might even argue
> > > > that only known table types should be forwarded in the first place,
> > > > regardless of the memory type.
> > >
> > > Which tables are worth handling in Xen?  I know about ACPI, SMBIOS, and
> > > ESRT, but I am curious which others Xen should preserve.  Currently, Xen
> > > does not know about RT_PROP or MEMATTR

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Thu, 6 Oct 2022 at 03:41, Demi Marie Obenour
 wrote:
>
> On Wed, Oct 05, 2022 at 11:28:29PM +0200, Ard Biesheuvel wrote:
> > On Wed, 5 Oct 2022 at 20:11, Demi Marie Obenour
> >  wrote:
> > >
> > > On Wed, Oct 05, 2022 at 08:15:07AM +0200, Jan Beulich wrote:
> > > > On 04.10.2022 17:46, Demi Marie Obenour wrote:
> > > > > Linux has a function called efi_mem_reserve() that is used to reserve
> > > > > EfiBootServicesData memory that contains e.g. EFI configuration 
> > > > > tables.
> > > > > This function does not work under Xen because Xen could have already
> > > > > clobbered the memory.  efi_mem_reserve() not working is the whole 
> > > > > reason
> > > > > for this thread, as it prevents EFI tables that are in
> > > > > EfiBootServicesData from being used under Xen.
> > > > >
> > > > > A much nicer approach would be for Xen to reserve boot services memory
> > > > > unconditionally, but provide a hypercall that dom0 could used to free
> > > > > the parts of EfiBootServicesData memory that are no longer needed.  
> > > > > This
> > > > > would allow efi_mem_reserve() to work normally.
> > > >
> > > > efi_mem_reserve() actually working would be a layering violation;
> > > > controlling the EFI memory map is entirely Xen's job.
> > >
> > > Doing this properly would require Xen to understand all of the EFI
> > > tables that could validly be in EfiBootServices* and which could be of
> > > interest to dom0.  It might (at least on some very buggy firmware)
> > > require a partial ACPI and/or SMBIOS implementation too, if the firmware
> > > decided to put an ACPI or SMBIOS table in EfiBootServices*.
> > >
> > > > As to the hypercall you suggest - I wouldn't mind its addition, but only
> > > > for the case when -mapbs is used. As I've indicated before, I'm of the
> > > > opinion that default behavior should be matching the intentions of the
> > > > spec, and the intention of EfiBootServices* is for the space to be
> > > > reclaimed. Plus I'm sure you realize there's a caveat with Dom0 using
> > > > that hypercall: It might use it for regions where data lives which it
> > > > wouldn't care about itself, but which an eventual kexec-ed (or alike)
> > > > entity would later want to consume. Code/data potentially usable by
> > > > _anyone_ between two resets of the system cannot legitimately be freed
> > > > (and hence imo is wrong to live in EfiBootServices* regions).
> > >
> > > I agree, but currently some such data *is* in EfiBootServices* regions,
> > > sadly.  When -mapbs is *not* used, I recommend uninstalling all of the
> > > configuration tables that point to EfiBootServicesData memory before
> > > freeing that memory.
> > >
> >
> > That seems like a reasonable approach to me. Tables like MEMATTR or
> > RT_PROP are mostly relevant for bare metal where the host kernel maps
> > the runtime services, and in general, passing on these tables without
> > knowing what they do is kind of fishy anyway. You might even argue
> > that only known table types should be forwarded in the first place,
> > regardless of the memory type.
>
> Which tables are worth handling in Xen?  I know about ACPI, SMBIOS, and
> ESRT, but I am curious which others Xen should preserve.  Currently, Xen
> does not know about RT_PROP or MEMATTR; could this be a cause of
> problems?

dom0 only has access to paravirtualized EFI runtime services, so
consuming RT_PROP or MEMATTR should be up to Xen (they describe which
runtime services remain available at runtime, and which permission
attributes to use for the runtime services memory regions,
respectively)

Looking through the kernel code, I don't think there are any that dom0
should care about beyond ACPI, SMBIOS and ESRT. But as you suggest,
that means Xen should just mask them in the view of the EFI system
table it exposes so dom0. Otherwise, the kernel may still try to map
and parse them.

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Wed, 5 Oct 2022 at 20:11, Demi Marie Obenour
 wrote:
>
> On Wed, Oct 05, 2022 at 08:15:07AM +0200, Jan Beulich wrote:
> > On 04.10.2022 17:46, Demi Marie Obenour wrote:
> > > Linux has a function called efi_mem_reserve() that is used to reserve
> > > EfiBootServicesData memory that contains e.g. EFI configuration tables.
> > > This function does not work under Xen because Xen could have already
> > > clobbered the memory.  efi_mem_reserve() not working is the whole reason
> > > for this thread, as it prevents EFI tables that are in
> > > EfiBootServicesData from being used under Xen.
> > >
> > > A much nicer approach would be for Xen to reserve boot services memory
> > > unconditionally, but provide a hypercall that dom0 could used to free
> > > the parts of EfiBootServicesData memory that are no longer needed.  This
> > > would allow efi_mem_reserve() to work normally.
> >
> > efi_mem_reserve() actually working would be a layering violation;
> > controlling the EFI memory map is entirely Xen's job.
>
> Doing this properly would require Xen to understand all of the EFI
> tables that could validly be in EfiBootServices* and which could be of
> interest to dom0.  It might (at least on some very buggy firmware)
> require a partial ACPI and/or SMBIOS implementation too, if the firmware
> decided to put an ACPI or SMBIOS table in EfiBootServices*.
>
> > As to the hypercall you suggest - I wouldn't mind its addition, but only
> > for the case when -mapbs is used. As I've indicated before, I'm of the
> > opinion that default behavior should be matching the intentions of the
> > spec, and the intention of EfiBootServices* is for the space to be
> > reclaimed. Plus I'm sure you realize there's a caveat with Dom0 using
> > that hypercall: It might use it for regions where data lives which it
> > wouldn't care about itself, but which an eventual kexec-ed (or alike)
> > entity would later want to consume. Code/data potentially usable by
> > _anyone_ between two resets of the system cannot legitimately be freed
> > (and hence imo is wrong to live in EfiBootServices* regions).
>
> I agree, but currently some such data *is* in EfiBootServices* regions,
> sadly.  When -mapbs is *not* used, I recommend uninstalling all of the
> configuration tables that point to EfiBootServicesData memory before
> freeing that memory.
>

That seems like a reasonable approach to me. Tables like MEMATTR or
RT_PROP are mostly relevant for bare metal where the host kernel maps
the runtime services, and in general, passing on these tables without
knowing what they do is kind of fishy anyway. You might even argue
that only known table types should be forwarded in the first place,
regardless of the memory type.

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
 wrote:
>
> On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> >  wrote:
> > >
> > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > This means that some sanity checks we would like to perform on
> > > > configuration tables or other data structures in memory are not
> > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > via a Xen hypercall, so let's wire that up instead.
> > > >
> > > > Co-developed-by: Demi Marie Obenour 
> > > > Signed-off-by: Demi Marie Obenour 
> > > > Signed-off-by: Ard Biesheuvel 
> > > > ---
> > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > >  drivers/xen/efi.c  | 34 
> > > >  include/linux/efi.h|  1 +
> > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > --- a/drivers/firmware/efi/efi.c
> > > > +++ b/drivers/firmware/efi/efi.c
> > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > >   * and if so, populate the supplied memory descriptor with the 
> > > > appropriate
> > > >   * data.
> > > >   */
> > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > >  {
> > > >   efi_memory_desc_t *md;
> > > >
> > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, 
> > > > efi_memory_desc_t *out_md)
> > > >   return -ENOENT;
> > > >  }
> > > >
> > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t 
> > > > *out_md)
> > > > +  __weak __alias(__efi_mem_desc_lookup);
> > > > +
> > > >  /*
> > > >   * Calculate the highest address of an efi memory descriptor.
> > > >   */
> > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > --- a/drivers/xen/efi.c
> > > > +++ b/drivers/xen/efi.c
> > > > @@ -26,6 +26,7 @@
> > > >
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >
> > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > >   efi.get_next_high_mono_count= 
> > > > xen_efi_get_next_high_mono_count;
> > > >   efi.reset_system= xen_efi_reset_system;
> > > >  }
> > > > +
> > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > +{
> > > > + static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > +   "Mismatch between EFI_PAGE_SHIFT and 
> > > > XEN_PAGE_SHIFT");
> > > > + struct xen_platform_op op = {
> > > > + .cmd = XENPF_firmware_info,
> > > > + .u.firmware_info = {
> > > > + .type = XEN_FW_EFI_INFO,
> > > > + .index = XEN_FW_EFI_MEM_INFO,
> > > > + .u.efi_info.mem.addr = phys_addr,
> > > > + .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > + }
> > > > + };
> > > > + union xenpf_efi_info *info = _info.u.efi_info;
> > > > + int rc;
> > > > +
> > > > + if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > + return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > +
> > > > + rc = HYPERVISOR_platform_op();
> > > > + if (rc) {
> > > > + pr_warn("Failed to lookup header 0x%llx in Xen memory 
> > > > map: error %d\n",
> > > > + phys_addr, rc);
> > > > + }
> > > > +
> > > > + out_md->phys_addr   = info->mem.addr;
> > >
> > > This will be equal to phys_addr, not the actual start of the memory
> > > regio

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
 wrote:
>
> On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > This means that some sanity checks we would like to perform on
> > configuration tables or other data structures in memory are not
> > currently possible. Xen does, however, expose EFI memory descriptor info
> > via a Xen hypercall, so let's wire that up instead.
> >
> > Co-developed-by: Demi Marie Obenour 
> > Signed-off-by: Demi Marie Obenour 
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  drivers/firmware/efi/efi.c |  5 ++-
> >  drivers/xen/efi.c  | 34 
> >  include/linux/efi.h|  1 +
> >  3 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 55bd3f4aab28..2c12b1a06481 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> >   * and if so, populate the supplied memory descriptor with the appropriate
> >   * data.
> >   */
> > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> >  {
> >   efi_memory_desc_t *md;
> >
> > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, 
> > efi_memory_desc_t *out_md)
> >   return -ENOENT;
> >  }
> >
> > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +  __weak __alias(__efi_mem_desc_lookup);
> > +
> >  /*
> >   * Calculate the highest address of an efi memory descriptor.
> >   */
> > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > --- a/drivers/xen/efi.c
> > +++ b/drivers/xen/efi.c
> > @@ -26,6 +26,7 @@
> >
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> >   efi.get_next_high_mono_count= xen_efi_get_next_high_mono_count;
> >   efi.reset_system= xen_efi_reset_system;
> >  }
> > +
> > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +{
> > + static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > +   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > + struct xen_platform_op op = {
> > + .cmd = XENPF_firmware_info,
> > + .u.firmware_info = {
> > + .type = XEN_FW_EFI_INFO,
> > + .index = XEN_FW_EFI_MEM_INFO,
> > + .u.efi_info.mem.addr = phys_addr,
> > + .u.efi_info.mem.size = U64_MAX - phys_addr,
> > + }
> > + };
> > + union xenpf_efi_info *info = _info.u.efi_info;
> > + int rc;
> > +
> > + if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > + return __efi_mem_desc_lookup(phys_addr, out_md);
> > +
> > + rc = HYPERVISOR_platform_op();
> > + if (rc) {
> > + pr_warn("Failed to lookup header 0x%llx in Xen memory map: 
> > error %d\n",
> > + phys_addr, rc);
> > + }
> > +
> > + out_md->phys_addr   = info->mem.addr;
>
> This will be equal to phys_addr, not the actual start of the memory
> region.
>
> > + out_md->num_pages   = info->mem.size >> EFI_PAGE_SHIFT;
>
> Similarly, this will be the number of bytes in the memory region
> after phys_addr, not the total number of bytes in the region.  These two
> differences mean that this function is not strictly equivalent to the
> original efi_mem_desc_lookup().
>
> I am not sure if this matters in practice, but I thought you would want
> to be aware of it.

This is a bit disappointing. Is there no way to obtain this
information via a Xen hypercall?

In any case, it means we'll need to round down phys_addr to page size
at the very least.

Re: [PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 17:18, Demi Marie Obenour
 wrote:
>
> On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote:
> > The ESRT code currently contains some sanity checks on the memory
> > descriptor it obtains, but these can only trigger when the descriptor is
> > invalid (if at all).
> >
> > So let's drop these checks, and instead, disregard descriptors entirely
> > if the start address is misaligned, or the number of pages reaches
> > beyond the end of the address space. Note that the memory map as a whole
> > could still be inconsistent, i.e., multiple entries might cover the same
> > area, or the address could be outside of the addressable VA space, but
> > validating that goes beyond the scope of these helpers.
> >
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  drivers/firmware/efi/efi.c  | 13 +++--
> >  drivers/firmware/efi/esrt.c | 18 +-
> >  2 files changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 11857af72859..55bd3f4aab28 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, 
> > efi_memory_desc_t *out_md)
> >   efi_memory_desc_t *md;
> >
> >   if (!efi_enabled(EFI_MEMMAP)) {
> > - pr_err_once("EFI_MEMMAP is not enabled.\n");
> > + pr_warn_once("EFI_MEMMAP is not enabled.\n");
> >   return -EINVAL;
> >   }
> >
> > - if (!out_md) {
> > - pr_err_once("out_md is null.\n");
> > - return -EINVAL;
> > -}
> > -
>
> Nit: this seems unrelated.
>
> >   for_each_efi_memory_desc(md) {
> >   u64 size;
> >   u64 end;
> >
> > + /* skip bogus entries */
> > + if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
> > + (md->phys_addr > 0 &&
> > +  (md->num_pages > (U64_MAX - md->phys_addr + 1) >> 
> > EFI_PAGE_SHIFT)))
> > + continue;
>
> Should this also check if md->num_pages is 0?

Yes, probably.

>  Also, should this check
> be part of for_each_efi_memory_desc()?
>

No, I don't think so. The for_each_xxx() helpers we have throughout
the kernel usually don't incorporate such checks, and I'd prefer to
adhere to the principle of least surprise here.

> > +
> >   size = md->num_pages << EFI_PAGE_SHIFT;
> >   end = md->phys_addr + size;
> >   if (phys_addr >= md->phys_addr && phys_addr < end) {
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index 2a2f52b017e7..8f86f2b0734b 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
> >   int rc;
> >   phys_addr_t end;
> >
> > - if (!efi_enabled(EFI_MEMMAP))
> > - return;
> > -
> >   pr_debug("esrt-init: loading.\n");
> >   if (!esrt_table_exists())
> >   return;
> > @@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
> >   return;
> >   }
> >
> > - max = efi_mem_desc_end();
> > - 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() - 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;
> > - }
>
> This can still happen if the ESRT pointer is very very close to the end
> of a memory map entry, unless there is another check that handles
> such cases.

You're right - I missed that.

[PATCH v2 6/6] efi: Apply allowlist to EFI configuration tables when running under Xen

[Ard Biesheuvel]

As it turns out, Xen does not guarantee that EFI bootservices data
regions in memory are preserved, which means that EFI configuration
tables pointing into such memory regions may be corrupted before the
dom0 OS has had a chance to inspect them.

Demi Marie reports that this is causing problems for Qubes OS when it
attempts to perform system firmware updates, which requires that the
contents of the ESRT configuration table are valid when the fwupd user
space program runs.

However, other configuration tables such as the memory attributes
table or the runtime properties table are equally affected, and so we
need a comprehensive workaround that works for any table type.

So when running under Xen, check the EFI memory descriptor covering the
start of the table, and disregard the table if it does not reside in
memory that is preserved by Xen.

Co-developed-by: Demi Marie Obenour 
Signed-off-by: Demi Marie Obenour 
Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/efi.c |  7 ++
 drivers/xen/efi.c  | 24 
 include/linux/efi.h|  2 ++
 3 files changed, 33 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 2c12b1a06481..0a4583c13a40 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -560,6 +560,13 @@ static __init int match_config_table(const efi_guid_t 
*guid,
 
for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
if (!efi_guidcmp(*guid, table_types[i].guid)) {
+   if (IS_ENABLED(CONFIG_XEN_EFI) &&
+   !xen_efi_config_table_is_usable(guid, table)) {
+   if (table_types[i].name[0])
+   pr_cont("(%s=0x%lx) ",
+   table_types[i].name, table);
+   return 1;
+   }
*(table_types[i].ptr) = table;
if (table_types[i].name[0])
pr_cont("%s=0x%lx ",
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 74f3f6d8cdc8..c275a9c377fe 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -326,3 +326,27 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t 
*out_md)
 
 return 0;
 }
+
+bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
+  unsigned long table)
+{
+   efi_memory_desc_t md;
+   int rc;
+
+   if (!efi_enabled(EFI_PARAVIRT))
+   return true;
+
+   rc = efi_mem_desc_lookup(table, );
+   if (rc)
+   return false;
+
+   switch (md.type) {
+   case EFI_RUNTIME_SERVICES_CODE:
+   case EFI_RUNTIME_SERVICES_DATA:
+   case EFI_ACPI_RECLAIM_MEMORY:
+   case EFI_RESERVED_TYPE:
+   return true;
+   }
+
+   return false;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e0ee6f6da4b4..b0cba86352ce 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1352,4 +1352,6 @@ struct linux_efi_initrd {
 /* Header of a populated EFI secret area */
 #define EFI_SECRET_TABLE_HEADER_GUID   EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  
0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
 
+bool xen_efi_config_table_is_usable(const efi_guid_t *, unsigned long table);
+
 #endif /* _LINUX_EFI_H */
-- 
2.35.1

[PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Ard Biesheuvel]

The ESRT code currently contains some sanity checks on the memory
descriptor it obtains, but these can only trigger when the descriptor is
invalid (if at all).

So let's drop these checks, and instead, disregard descriptors entirely
if the start address is misaligned, or the number of pages reaches
beyond the end of the address space. Note that the memory map as a whole
could still be inconsistent, i.e., multiple entries might cover the same
area, or the address could be outside of the addressable VA space, but
validating that goes beyond the scope of these helpers.

Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/efi.c  | 13 +++--
 drivers/firmware/efi/esrt.c | 18 +-
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 11857af72859..55bd3f4aab28 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t 
*out_md)
efi_memory_desc_t *md;
 
if (!efi_enabled(EFI_MEMMAP)) {
-   pr_err_once("EFI_MEMMAP is not enabled.\n");
+   pr_warn_once("EFI_MEMMAP is not enabled.\n");
return -EINVAL;
}
 
-   if (!out_md) {
-   pr_err_once("out_md is null.\n");
-   return -EINVAL;
-}
-
for_each_efi_memory_desc(md) {
u64 size;
u64 end;
 
+   /* skip bogus entries */
+   if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
+   (md->phys_addr > 0 &&
+(md->num_pages > (U64_MAX - md->phys_addr + 1) >> 
EFI_PAGE_SHIFT)))
+   continue;
+
size = md->num_pages << EFI_PAGE_SHIFT;
end = md->phys_addr + size;
if (phys_addr >= md->phys_addr && phys_addr < end) {
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e7..8f86f2b0734b 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
int rc;
phys_addr_t end;
 
-   if (!efi_enabled(EFI_MEMMAP))
-   return;
-
pr_debug("esrt-init: loading.\n");
if (!esrt_table_exists())
return;
@@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
return;
}
 
-   max = efi_mem_desc_end();
-   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() - 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;
-   }
 
va = early_memremap(efi.esrt, size);
if (!va) {
-- 
2.35.1

[PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

Xen on x86 boots dom0 in EFI mode but without providing a memory map.
This means that some sanity checks we would like to perform on
configuration tables or other data structures in memory are not
currently possible. Xen does, however, expose EFI memory descriptor info
via a Xen hypercall, so let's wire that up instead.

Co-developed-by: Demi Marie Obenour 
Signed-off-by: Demi Marie Obenour 
Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/efi.c |  5 ++-
 drivers/xen/efi.c  | 34 
 include/linux/efi.h|  1 +
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 55bd3f4aab28..2c12b1a06481 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
  * and if so, populate the supplied memory descriptor with the appropriate
  * data.
  */
-int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 {
efi_memory_desc_t *md;
 
@@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t 
*out_md)
return -ENOENT;
 }
 
+extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+__weak __alias(__efi_mem_desc_lookup);
+
 /*
  * Calculate the highest address of an efi memory descriptor.
  */
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index d1ff2186ebb4..74f3f6d8cdc8 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -26,6 +26,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
efi.get_next_high_mono_count= xen_efi_get_next_high_mono_count;
efi.reset_system= xen_efi_reset_system;
 }
+
+int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+{
+   static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
+ "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
+   struct xen_platform_op op = {
+   .cmd = XENPF_firmware_info,
+   .u.firmware_info = {
+   .type = XEN_FW_EFI_INFO,
+   .index = XEN_FW_EFI_MEM_INFO,
+   .u.efi_info.mem.addr = phys_addr,
+   .u.efi_info.mem.size = U64_MAX - phys_addr,
+   }
+   };
+   union xenpf_efi_info *info = _info.u.efi_info;
+   int rc;
+
+   if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
+   return __efi_mem_desc_lookup(phys_addr, out_md);
+
+   rc = HYPERVISOR_platform_op();
+   if (rc) {
+   pr_warn("Failed to lookup header 0x%llx in Xen memory map: 
error %d\n",
+   phys_addr, rc);
+   }
+
+   out_md->phys_addr   = info->mem.addr;
+   out_md->num_pages   = info->mem.size >> EFI_PAGE_SHIFT;
+   out_md->type= info->mem.type;
+   out_md->attribute   = info->mem.attr;
+
+return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 256e70e42114..e0ee6f6da4b4 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -731,6 +731,7 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, 
unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern u64 efi_mem_desc_end(efi_memory_desc_t *md);
 extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
+extern int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
 extern void efi_mem_reserve(phys_addr_t addr, u64 size);
 extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
-- 
2.35.1

[PATCH v2 2/6] efi: memmap: Move manipulation routines into x86 arch tree

[Ard Biesheuvel]

The EFI memory map is a description of the memory layout as provided by
the firmware, and only x86 manipulates it in various different ways for
its own memory bookkeeping. So let's move the memmap routines that are
only used by x86 into the x86 arch tree.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/include/asm/efi.h |  11 +
 arch/x86/platform/efi/Makefile |   3 +-
 arch/x86/platform/efi/memmap.c | 235 
 drivers/firmware/efi/memmap.c  | 221 +-
 include/linux/efi.h|  10 +-
 5 files changed, 251 insertions(+), 229 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 68414d924332..1fb4686f3d12 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -415,6 +415,17 @@ static inline void efi_fake_memmap(void)
 }
 #endif
 
+extern int __init efi_memmap_alloc(unsigned int num_entries,
+  struct efi_memory_map_data *data);
+extern void __efi_memmap_free(u64 phys, unsigned long size,
+ unsigned long flags);
+
+extern int __init efi_memmap_install(struct efi_memory_map_data *data);
+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);
+
 #define arch_ima_efi_boot_mode \
({ extern struct boot_params boot_params; boot_params.secure_boot; })
 
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index b481719b16cc..ed5502a5185d 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,6 +2,7 @@
 KASAN_SANITIZE := n
 GCOV_PROFILE := n
 
-obj-$(CONFIG_EFI)  += quirks.o efi.o efi_$(BITS).o 
efi_stub_$(BITS).o
+obj-$(CONFIG_EFI)  += memmap.o quirks.o efi.o efi_$(BITS).o \
+  efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)+= efi_thunk_$(BITS).o
 obj-$(CONFIG_EFI_FAKE_MEMMAP)  += fake_mem.o
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
new file mode 100644
index ..44b886acf301
--- /dev/null
+++ b/arch/x86/platform/efi/memmap.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common EFI memory map functions.
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
+{
+   return memblock_phys_alloc(size, SMP_CACHE_BYTES);
+}
+
+static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
+{
+   unsigned int order = get_order(size);
+   struct page *p = alloc_pages(GFP_KERNEL, order);
+
+   if (!p)
+   return 0;
+
+   return PFN_PHYS(page_to_pfn(p));
+}
+
+void __init __efi_memmap_free(u64 phys, unsigned long size, unsigned long 
flags)
+{
+   if (flags & EFI_MEMMAP_MEMBLOCK) {
+   if (slab_is_available())
+   memblock_free_late(phys, size);
+   else
+   memblock_phys_free(phys, size);
+   } else if (flags & EFI_MEMMAP_SLAB) {
+   struct page *p = pfn_to_page(PHYS_PFN(phys));
+   unsigned int order = get_order(size);
+
+   free_pages((unsigned long) page_address(p), order);
+   }
+}
+
+/**
+ * efi_memmap_alloc - Allocate memory for the EFI memory map
+ * @num_entries: Number of entries in the allocated map.
+ * @data: efi memmap installation parameters
+ *
+ * Depending on whether mm_init() has already been invoked or not,
+ * either memblock or "normal" page allocation is used.
+ *
+ * Returns zero on success, a negative error code on failure.
+ */
+int __init efi_memmap_alloc(unsigned int num_entries,
+   struct efi_memory_map_data *data)
+{
+   /* Expect allocation parameters are zero initialized */
+   WARN_ON(data->phys_map || data->size);
+
+   data->size = num_entries * efi.memmap.desc_size;
+   data->desc_version = efi.memmap.desc_version;
+   data->desc_size = efi.memmap.desc_size;
+   data->flags &= ~(EFI_MEMMAP_SLAB | EFI_MEMMAP_MEMBLOCK);
+   data->flags |= efi.memmap.flags & EFI_MEMMAP_LATE;
+
+   if (slab_is_available()) {
+   data->flags |= EFI_MEMMAP_SLAB;
+   data->phys_map = __efi_memmap_alloc_late(data->size);
+   } else {
+   data->flags |= EFI_MEMMAP_MEMBLOCK;
+   data->phys_map = __efi_memmap_alloc_early(data->size);
+   }
+
+   if (!data->phys_map)
+   return -ENOMEM;
+   return 0;
+}
+
+/**
+ * efi_memmap_install - Install a new EFI memory map in efi.memmap
+ * @ctx: map allocation parameters (address, size, flags)
+ *
+ * Unlike efi_memmap_init_*(), thi

[PATCH v2 1/6] efi: Move EFI fake memmap support into x86 arch tree

[Ard Biesheuvel]

The EFI fake memmap support is specific to x86, which manipulates the
EFI memory map in various different ways after receiving it from the EFI
stub. On other architectures, we have manages to push back on this, and
the EFI memory map is kept pristine.

So let's move the fake memmap code into the x86 arch tree, where it
arguably belongs.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/Kconfig   | 20 +
 arch/x86/include/asm/efi.h |  5 ++
 arch/x86/kernel/setup.c|  1 +
 arch/x86/platform/efi/Makefile |  1 +
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c | 79 
+++-
 drivers/firmware/efi/Kconfig   | 22 --
 drivers/firmware/efi/Makefile  |  4 -
 drivers/firmware/efi/fake_mem.h| 10 ---
 drivers/firmware/efi/x86_fake_mem.c| 75 ---
 include/linux/efi.h|  6 --
 10 files changed, 103 insertions(+), 120 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..b98941c2fec4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1978,6 +1978,26 @@ config EFI_MIXED
 
  If unsure, say N.
 
+config EFI_FAKE_MEMMAP
+   bool "Enable EFI fake memory map"
+   depends on EFI
+   help
+ Saying Y here will enable "efi_fake_mem" boot option.  By specifying
+ this parameter, you can add arbitrary attribute to specific memory
+ range by updating original (firmware provided) EFI memmap.  This is
+ useful for debugging of EFI memmap related feature, e.g., Address
+ Range Mirroring feature.
+
+config EFI_MAX_FAKE_MEM
+   int "maximum allowable number of ranges in efi_fake_mem boot option"
+   depends on EFI_FAKE_MEMMAP
+   range 1 128
+   default 8
+   help
+ Maximum allowable number of ranges in efi_fake_mem boot option.
+ Ranges can be set up to this value using comma-separated list.
+ The default value is 8.
+
 source "kernel/Kconfig.hz"
 
 config KEXEC
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 897ea4aec16e..68414d924332 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -404,10 +404,15 @@ static inline void efi_reserve_boot_services(void)
 
 #ifdef CONFIG_EFI_FAKE_MEMMAP
 extern void __init efi_fake_memmap_early(void);
+extern void __init efi_fake_memmap(void);
 #else
 static inline void efi_fake_memmap_early(void)
 {
 }
+
+static inline void efi_fake_memmap(void)
+{
+}
 #endif
 
 #define arch_ima_efi_boot_mode \
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7144ee..41ec3a69f3c7 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -31,6 +31,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index a50245157685..b481719b16cc 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -4,3 +4,4 @@ GCOV_PROFILE := n
 
 obj-$(CONFIG_EFI)  += quirks.o efi.o efi_$(BITS).o 
efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_FAKE_MEMMAP)  += fake_mem.o
diff --git a/drivers/firmware/efi/fake_mem.c b/arch/x86/platform/efi/fake_mem.c
similarity index 58%
rename from drivers/firmware/efi/fake_mem.c
rename to arch/x86/platform/efi/fake_mem.c
index 6e0f34a38171..41d57cad3d84 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/arch/x86/platform/efi/fake_mem.c
@@ -17,10 +17,13 @@
 #include 
 #include 
 #include 
-#include "fake_mem.h"
+#include 
+#include 
 
-struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
-int nr_fake_mem;
+#define EFI_MAX_FAKEMEM CONFIG_EFI_MAX_FAKE_MEM
+
+static struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
+static int nr_fake_mem;
 
 static int __init cmp_fake_mem(const void *x1, const void *x2)
 {
@@ -122,3 +125,73 @@ static int __init setup_fake_mem(char *p)
 }
 
 early_param("efi_fake_mem", setup_fake_mem);
+
+void __init efi_fake_memmap_early(void)
+{
+   int i;
+
+   /*
+* The late efi_fake_mem() call can handle all requests if
+* EFI_MEMORY_SP support is disabled.
+*/
+   if (!efi_soft_reserve_enabled())
+   return;
+
+   if (!efi_enabled(EFI_MEMMAP) || !nr_fake_mem)
+   return;
+
+   /*
+* Given that efi_fake_memmap() needs to perform memblock
+* allocations it needs to run after e820__memblock_setup().
+* However, if efi_fake_mem specifies EFI_MEMORY_SP for a given
+* address range that potentially needs to mark the memory as
+* reserved prior to e820__memblock_setup(). Update e820
+* directly if EFI_MEMORY_SP is

[PATCH v2 3/6] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures

[Ard Biesheuvel]

Currently, the EFI_PARAVIRT flag is only used by x86, even though other
architectures also support pseudo-EFI boot, where the core kernel is
invoked directly and provided with a set of data tables that resemble
the ones constructed by the EFI stub, which never actually runs in that
case.

Let's fix this inconsistency, and always set this flag when booting dom0
via the EFI boot path. Note that Xen on x86 does not provide the EFI
memory map in this case, whereas other architectures do, so move the
associated EFI_PARAVIRT check into the x86 platform code.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/platform/efi/efi.c  | 8 +---
 arch/x86/platform/efi/memmap.c   | 3 +++
 drivers/firmware/efi/fdtparams.c | 4 
 drivers/firmware/efi/memmap.c| 3 ---
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 6e598bd78eef..6a6f2a585a3d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -214,9 +214,11 @@ int __init efi_memblock_x86_reserve_range(void)
data.desc_size  = e->efi_memdesc_size;
data.desc_version   = e->efi_memdesc_version;
 
-   rv = efi_memmap_init_early();
-   if (rv)
-   return rv;
+   if (!efi_enabled(EFI_PARAVIRT)) {
+   rv = efi_memmap_init_early();
+   if (rv)
+   return rv;
+   }
 
if (add_efi_memmap || do_efi_soft_reserve())
do_add_efi_memmap();
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
index 44b886acf301..18e14ec16720 100644
--- a/arch/x86/platform/efi/memmap.c
+++ b/arch/x86/platform/efi/memmap.c
@@ -93,6 +93,9 @@ int __init efi_memmap_install(struct efi_memory_map_data 
*data)
 {
efi_memmap_unmap();
 
+   if (efi_enabled(EFI_PARAVIRT))
+   return 0;
+
return __efi_memmap_init(data);
 }
 
diff --git a/drivers/firmware/efi/fdtparams.c b/drivers/firmware/efi/fdtparams.c
index e901f8564ca0..0ec83ba58097 100644
--- a/drivers/firmware/efi/fdtparams.c
+++ b/drivers/firmware/efi/fdtparams.c
@@ -30,11 +30,13 @@ static __initconst const char name[][22] = {
 
 static __initconst const struct {
const char  path[17];
+   u8  paravirt;
const char  params[PARAMCOUNT][26];
 } dt_params[] = {
{
 #ifdef CONFIG_XEN//  <---17-->
.path = "/hypervisor/uefi",
+   .paravirt = 1,
.params = {
[SYSTAB] = "xen,uefi-system-table",
[MMBASE] = "xen,uefi-mmap-start",
@@ -121,6 +123,8 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data 
*mm)
pr_err("Can't find property '%s' in DT!\n", pname);
return 0;
}
+   if (dt_params[i].paravirt)
+   set_bit(EFI_PARAVIRT, );
return systab;
}
 notfound:
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 3501d3814f22..9508082af907 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -44,9 +44,6 @@ int __init __efi_memmap_init(struct efi_memory_map_data *data)
struct efi_memory_map map;
phys_addr_t phys_map;
 
-   if (efi_enabled(EFI_PARAVIRT))
-   return 0;
-
phys_map = data->phys_map;
 
if (data->flags & EFI_MEMMAP_LATE)
-- 
2.35.1

[PATCH v2 0/6] efi/x86: Avoid corrupted config tables under Xen

[Ard Biesheuvel]

This is an alternate approach to addressing the issue that Demi Marie is
attempting to fix in [0] (i.e., ESRT config table exposed to a x86 dom0
is corrupted because it resides in boot services memory as per the EFI
spec, where it gets corrupted by Xen). My main objection to that approach
is that it needs Xen-specific fixes in multiple different places, but we
still end up only fixing the ESRT case specifically.

So instead, I am proposing this series as a more generic way to handle
configuration tables that reside in boot services memory, and confining
the Xen specific logic to the Xen EFI glue code.

Given that EFI boot without a memory map is only permitted on x86 and
only when doing Xen boot, let's clear up some inconsistencies there
first so we can set the EFI_PARAVIRT flag on all architectures that do
pseudo-EFI boot straight into the core kernel (i.e., without going
through the stub). This moves a good chunk of EFI memory map
manipulation code into the x86 arch tree, where it arguably belongs as
no other architectures rely on it. This is implemented in patches 1 - 3.

Patch #4 refactors the ESRT sanity checks on the memory descriptor, by
moving them into the efi_mem_desc_lookup() helper, which should not
return corrupted descriptors in the first place.

Patch #5 adds a Xen hypercall fallback to efi_mem_desc_lookup() when
running under Xen without a EFI memory map, so that, e.g., the existing
ESRT code will perform its validation against the Xen provided
descriptor if no memory map is available.

Patch #6 updates the config table traversal code so that the Xen glue
code can force them to be disregarded, which happens when the table in
question points into a memory region that is not of a type that Xen
automatically reserves. Future changes can refine this logic if needed.

Changes since v1:
- add patch #4
- move Xen descriptor lookup into efi_mem_desc_lookup()
- drop allowlist for ACPI and SMBIOS tables

[0] 
https://lore.kernel.org/all/cover.1664298147.git.d...@invisiblethingslab.com/

Cc: Demi Marie Obenour 
Cc: Peter Jones 
Cc: Juergen Gross 
Cc: Stefano Stabellini 
Cc: Oleksandr Tyshchenko 
Cc: Kees Cook 
Cc: Anton Vorontsov 
Cc: Colin Cross 
Cc: Tony Luck 
Cc: Marek Marczykowski-Górecki 

Ard Biesheuvel (6):
  efi: Move EFI fake memmap support into x86 arch tree
  efi: memmap: Move manipulation routines into x86 arch tree
  efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures
  efi: memmap: Disregard bogus entries instead of returning them
  efi: xen: Implement memory descriptor lookup based on hypercall
  efi: Apply allowlist to EFI configuration tables when running under
Xen

 arch/x86/Kconfig   |  20 ++
 arch/x86/include/asm/efi.h |  16 ++
 arch/x86/kernel/setup.c|   1 +
 arch/x86/platform/efi/Makefile |   4 +-
 arch/x86/platform/efi/efi.c|   8 +-
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c |  79 ++-
 arch/x86/platform/efi/memmap.c | 238 

 drivers/firmware/efi/Kconfig   |  22 --
 drivers/firmware/efi/Makefile  |   4 -
 drivers/firmware/efi/efi.c |  25 +-
 drivers/firmware/efi/esrt.c|  18 +-
 drivers/firmware/efi/fake_mem.h|  10 -
 drivers/firmware/efi/fdtparams.c   |   4 +
 drivers/firmware/efi/memmap.c  | 224 +-
 drivers/firmware/efi/x86_fake_mem.c|  75 --
 drivers/xen/efi.c  |  58 +
 include/linux/efi.h|  19 +-
 17 files changed, 446 insertions(+), 379 deletions(-)
 rename {drivers/firmware => arch/x86/platform}/efi/fake_mem.c (58%)
 create mode 100644 arch/x86/platform/efi/memmap.c
 delete mode 100644 drivers/firmware/efi/fake_mem.h
 delete mode 100644 drivers/firmware/efi/x86_fake_mem.c

-- 
2.35.1

Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available

[Ard Biesheuvel]

On Sun, 2 Oct 2022 at 23:43, Ard Biesheuvel  wrote:
>
> On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
>  wrote:
> >
> > On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> > > In order to permit the ESRT to be used when doing pseudo-EFI boot
> > > without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> > > make the sanity checks optional based on whether the memory map is
> > > available.
> > >
> > > If additional validation is needed, it is up to the Xen EFI glue code to
> > > implement this in its xen_efi_config_table_is_valid() helper, or provide
> > > a EFI memory map like it does on other architectures.
> >
> > I don’t like this.  It is easy to use a hypercall to get the end of the
> > memory region containing the config table, which is what my one of my
> > previous patches actually does.  Skipping all of the validation could
> > easily lead to a regression.
>
> I don't like putting Xen specific hacks left and right because Xen on
> x86 cannot be bothered to provide an EFI memory map. And as for
> regressions, ESRT does not work at all under Xen (given the lack of a
> memory map) and so I fail to see how this could break a currently
> working case.
>
> >  I understand wanting to get Xen-specific
> > code out of esrt.c, but this isn’t the answer.  Some sort of abstraction
> > over both cases would be a much better solution.
> >
>
> We have such an abstraction already, it is called the EFI memory map.
>
> So there are two options here:
> - expose a EFI memory map
> - add a ESRT specific check to xen_efi_config_table_is_valid() so that
> the ESRT is withheld from dom0 if there is something wrong with it.
>

Actually, the obvious answer here is to implement
efi_mem_desc_lookup() for the EFI_PARAVIRT / !EFI_MEMMAP case. I'll
have a go at that and send a v2 shortly.

Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available

[Ard Biesheuvel]

On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
 wrote:
>
> On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> > In order to permit the ESRT to be used when doing pseudo-EFI boot
> > without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> > make the sanity checks optional based on whether the memory map is
> > available.
> >
> > If additional validation is needed, it is up to the Xen EFI glue code to
> > implement this in its xen_efi_config_table_is_valid() helper, or provide
> > a EFI memory map like it does on other architectures.
>
> I don’t like this.  It is easy to use a hypercall to get the end of the
> memory region containing the config table, which is what my one of my
> previous patches actually does.  Skipping all of the validation could
> easily lead to a regression.

I don't like putting Xen specific hacks left and right because Xen on
x86 cannot be bothered to provide an EFI memory map. And as for
regressions, ESRT does not work at all under Xen (given the lack of a
memory map) and so I fail to see how this could break a currently
working case.

>  I understand wanting to get Xen-specific
> code out of esrt.c, but this isn’t the answer.  Some sort of abstraction
> over both cases would be a much better solution.
>

We have such an abstraction already, it is called the EFI memory map.

So there are two options here:
- expose a EFI memory map
- add a ESRT specific check to xen_efi_config_table_is_valid() so that
the ESRT is withheld from dom0 if there is something wrong with it.

And frankly, the validation itself could use some attention as well:

"""
rc = efi_mem_desc_lookup(efi.esrt, );
...
max = efi_mem_desc_end();
if (max < efi.esrt) {
"""

Unless I am missing something, this can never occur so the check is
pointless and the pr_err() that follows is unreachable.

Then we have

"""
size = sizeof(*esrt);
max -= efi.esrt;

if (max < size) {
"""

'size' is 16 bytes here, so the only way this can become true is if
the memory descriptor describes a region of 0 pages in length, which
is explicitly forbidden by the EFI spec. If such a descriptor exists
in spite of that, this is a memory map problem not a ESRT problem.

So actually, instead of making these checks conditional on EFI_MEMMAP
being set, I might just rip them out entirely and be done with it.



> > Co-developed-by: Demi Marie Obenour 
> > Signed-off-by: Demi Marie Obenour 
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  arch/x86/platform/efi/quirks.c |  3 +
> >  drivers/firmware/efi/esrt.c| 61 +++-
> >  2 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index b0b848d6933a..9307be2f4afa 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 
> > size)
> >   int num_entries;
> >   void *new;
> >
> > + if (!efi_enabled(EFI_MEMMAP))
> > + return;
> > +
>
> This function does not actually work under Xen, even if EFI_MEMMAP is
> set.  When running under Xen, either this function must never be
> called (in which case there should be at least a WARN()), or it should
> return an error that callers must check for.
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

Re: [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen

[Ard Biesheuvel]

On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
 wrote:
>
> On Sun, Oct 02, 2022 at 11:56:25AM +0200, Ard Biesheuvel wrote:
> > As it turns out, Xen does not guarantee that EFI bootservices data
> > regions in memory are preserved, which means that EFI configuration
> > tables pointing into such memory regions may be corrupted before the
> > dom0 OS has had a chance to inspect them.
> >
> > Demi Marie reports that this is causing problems for Qubes OS when it
> > attempts to perform system firmware updates, which requires that the
> > contents of the ESRT configuration table are valid when the fwupd user
> > space program runs.
> >
> > However, other configuration tables such as the memory attributes
> > table or the runtime properties table are equally affected, and so we
> > need a comprehensive workaround that works for any table type.
> >
> > So let's first disregard all EFI configuration tables except the ones
> > that are known (or can be expected) to reside in memory regions of a
> > type that Xen preserves, i.e., ACPI tables (which are passed in
> > EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
> > passed in EfiRuntimeServicesData regions, even though the UEFI spec only
> > mentions this as a recommendation). Then, cross reference unknown tables
> > against either the EFI memory map (if available) or do a Xen hypercall
> > to determine the memory type, and allow the config table if the type is
> > one that is guaranteed to be preserved.
> >
> > Future patches can augment the logic in this routine to allow other
> > table types based on the size of the allocation, or based on a table
> > specific header size field.
> >
> > Co-developed-by: Demi Marie Obenour 
> > Signed-off-by: Demi Marie Obenour 
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  drivers/firmware/efi/efi.c |  7 ++
> >  drivers/xen/efi.c  | 69 
> >  include/linux/efi.h|  2 +
> >  3 files changed, 78 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 11857af72859..e8c0747011d7 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -556,6 +556,13 @@ static __init int match_config_table(const efi_guid_t 
> > *guid,
> >
> >   for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
> >   if (!efi_guidcmp(*guid, table_types[i].guid)) {
> > + if (IS_ENABLED(CONFIG_XEN_EFI) &&
> > + !xen_efi_config_table_is_usable(guid, table)) {
> > + if (table_types[i].name[0])
> > + pr_cont("(%s=0x%lx) ",
> > + table_types[i].name, table);
> > + return 1;
> > + }
> >   *(table_types[i].ptr) = table;
> >   if (table_types[i].name[0])
> >   pr_cont("%s=0x%lx ",
> > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > index d1ff2186ebb4..3f1f365b37d0 100644
> > --- a/drivers/xen/efi.c
> > +++ b/drivers/xen/efi.c
> > @@ -292,3 +292,72 @@ void __init xen_efi_runtime_setup(void)
> >   efi.get_next_high_mono_count= xen_efi_get_next_high_mono_count;
> >   efi.reset_system= xen_efi_reset_system;
> >  }
> > +
> > +static const efi_guid_t cfg_table_allow_list[] __initconst = {
> > + ACPI_20_TABLE_GUID,
> > + ACPI_TABLE_GUID,
> > + SMBIOS_TABLE_GUID,
> > + SMBIOS3_TABLE_GUID,
> > +};
>
> This allowlist seems redundant.  Either the tables are already in memory
> that Xen will preserve or they aren’t.  In both cases the subsequent
> code will do the right thing.

Will it? Currently, Xen simply accepts all ACPI and SMBIOS tables,
regardless of what type of memory region they reside in (if any).

So what will happen with buggy firmware where the ACPI or SMBIOS
tables are not covered by the memory map at all? Currently, this works
fine but now, it will be rejected. And without ACPI tables, the boot
will not get far enough to even inform the user what is wrong. And
SMBIOS tables are used for platform quirks, which means they might be
essential for a platform to boot as well.

[RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available

[Ard Biesheuvel]

In order to permit the ESRT to be used when doing pseudo-EFI boot
without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
make the sanity checks optional based on whether the memory map is
available.

If additional validation is needed, it is up to the Xen EFI glue code to
implement this in its xen_efi_config_table_is_valid() helper, or provide
a EFI memory map like it does on other architectures.

Co-developed-by: Demi Marie Obenour 
Signed-off-by: Demi Marie Obenour 
Signed-off-by: Ard Biesheuvel 
---
 arch/x86/platform/efi/quirks.c |  3 +
 drivers/firmware/efi/esrt.c| 61 +++-
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index b0b848d6933a..9307be2f4afa 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
int num_entries;
void *new;
 
+   if (!efi_enabled(EFI_MEMMAP))
+   return;
+
if (efi_mem_desc_lookup(addr, ) ||
md.type != EFI_BOOT_SERVICES_DATA) {
pr_err("Failed to lookup EFI memory descriptor for %pa\n", 
);
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e7..adb31fba45ae 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -243,40 +243,45 @@ void __init efi_esrt_init(void)
void *va;
struct efi_system_resource_table tmpesrt;
size_t size, max, entry_size, entries_size;
-   efi_memory_desc_t md;
-   int rc;
+   bool reserve_esrt;
phys_addr_t end;
 
-   if (!efi_enabled(EFI_MEMMAP))
-   return;
-
pr_debug("esrt-init: loading.\n");
if (!esrt_table_exists())
return;
 
-   rc = efi_mem_desc_lookup(efi.esrt, );
-   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;
-   }
+   size = sizeof(*esrt);
+   if (efi_enabled(EFI_MEMMAP)) {
+   efi_memory_desc_t md;
+   int rc;
+
+   rc = efi_mem_desc_lookup(efi.esrt, );
+   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();
-   if (max < efi.esrt) {
-   pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
-  (void *)efi.esrt, (void *)max);
-   return;
-   }
+   reserve_esrt = (md.type == EFI_BOOT_SERVICES_DATA);
+   max = efi_mem_desc_end();
+   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;
+   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;
+   }
+   } else {
+   reserve_esrt = true;
+   max = SIZE_MAX;
}
 
va = early_memremap(efi.esrt, size);
@@ -332,9 +337,11 @@ void __init efi_esrt_init(void)
esrt_data_size = size;
 
end = esrt_data + size;
-   pr_info("Reserving ESRT space from %pa to %pa.\n", _data, );
-   if (md.type == EFI_BOOT_SERVICES_DATA)
+   if (reserve_esrt) {
+   pr_info("Reserving ESRT space from %pa to %pa.\n", _data,
+   );
efi_mem_reserve(esrt_data, esrt_data_size);
+   }
 
pr_debug("esrt-init: loaded.\n");
 }
-- 
2.35.1

[RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen

[Ard Biesheuvel]

As it turns out, Xen does not guarantee that EFI bootservices data
regions in memory are preserved, which means that EFI configuration
tables pointing into such memory regions may be corrupted before the
dom0 OS has had a chance to inspect them.

Demi Marie reports that this is causing problems for Qubes OS when it
attempts to perform system firmware updates, which requires that the
contents of the ESRT configuration table are valid when the fwupd user
space program runs.

However, other configuration tables such as the memory attributes
table or the runtime properties table are equally affected, and so we
need a comprehensive workaround that works for any table type.

So let's first disregard all EFI configuration tables except the ones
that are known (or can be expected) to reside in memory regions of a
type that Xen preserves, i.e., ACPI tables (which are passed in
EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
passed in EfiRuntimeServicesData regions, even though the UEFI spec only
mentions this as a recommendation). Then, cross reference unknown tables
against either the EFI memory map (if available) or do a Xen hypercall
to determine the memory type, and allow the config table if the type is
one that is guaranteed to be preserved.

Future patches can augment the logic in this routine to allow other
table types based on the size of the allocation, or based on a table
specific header size field.

Co-developed-by: Demi Marie Obenour 
Signed-off-by: Demi Marie Obenour 
Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/efi.c |  7 ++
 drivers/xen/efi.c  | 69 
 include/linux/efi.h|  2 +
 3 files changed, 78 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 11857af72859..e8c0747011d7 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -556,6 +556,13 @@ static __init int match_config_table(const efi_guid_t 
*guid,
 
for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
if (!efi_guidcmp(*guid, table_types[i].guid)) {
+   if (IS_ENABLED(CONFIG_XEN_EFI) &&
+   !xen_efi_config_table_is_usable(guid, table)) {
+   if (table_types[i].name[0])
+   pr_cont("(%s=0x%lx) ",
+   table_types[i].name, table);
+   return 1;
+   }
*(table_types[i].ptr) = table;
if (table_types[i].name[0])
pr_cont("%s=0x%lx ",
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index d1ff2186ebb4..3f1f365b37d0 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -292,3 +292,72 @@ void __init xen_efi_runtime_setup(void)
efi.get_next_high_mono_count= xen_efi_get_next_high_mono_count;
efi.reset_system= xen_efi_reset_system;
 }
+
+static const efi_guid_t cfg_table_allow_list[] __initconst = {
+   ACPI_20_TABLE_GUID,
+   ACPI_TABLE_GUID,
+   SMBIOS_TABLE_GUID,
+   SMBIOS3_TABLE_GUID,
+};
+
+bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
+  unsigned long table)
+{
+   u32 memtype;
+   int i, rc;
+
+   if (!efi_enabled(EFI_PARAVIRT))
+   return true;
+
+   for (i = 0; i < ARRAY_SIZE(cfg_table_allow_list); i++) {
+   if (!efi_guidcmp(*guid, cfg_table_allow_list[i]))
+   return true;
+   }
+
+   if (efi_enabled(EFI_MEMMAP)) {
+   /* check against the EFI memory map */
+   efi_memory_desc_t md;
+
+   rc = efi_mem_desc_lookup(table, );
+   if (rc) {
+   pr_warn("Failed to lookup header 0x%lx in EFI memory 
map (%d)\n",
+   table, rc);
+   return false;
+   }
+   memtype = md.type;
+   } else {
+   /* check against the Xen hypercall */
+   struct xen_platform_op op = {
+   .cmd = XENPF_firmware_info,
+   .u.firmware_info = {
+   .type = XEN_FW_EFI_INFO,
+   .index = XEN_FW_EFI_MEM_INFO,
+   .u.efi_info.mem.addr = table,
+   .u.efi_info.mem.size = U64_MAX - table,
+   }
+   };
+   union xenpf_efi_info *info = _info.u.efi_info;
+
+   rc = HYPERVISOR_platform_op();
+   if (rc) {
+   pr_warn("Failed to lookup header 0x%lx in Xen memory 
map (%d)\n",
+   table, rc);
+   return false;
+   }
+   memtype = info->mem.typ

[RFC PATCH 2/5] efi: memmap: Move manipulation routines into x86 arch tree

[Ard Biesheuvel]

The EFI memory map is a description of the memory layout as provided by
the firmware, and only x86 manipulates it in various different ways for
its own memory bookkeeping. So let's move the memmap routines that are
only used by x86 into the x86 arch tree.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/include/asm/efi.h |  11 +
 arch/x86/platform/efi/Makefile |   3 +-
 arch/x86/platform/efi/memmap.c | 235 
 drivers/firmware/efi/memmap.c  | 221 +-
 include/linux/efi.h|  10 +-
 5 files changed, 251 insertions(+), 229 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 68414d924332..1fb4686f3d12 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -415,6 +415,17 @@ static inline void efi_fake_memmap(void)
 }
 #endif
 
+extern int __init efi_memmap_alloc(unsigned int num_entries,
+  struct efi_memory_map_data *data);
+extern void __efi_memmap_free(u64 phys, unsigned long size,
+ unsigned long flags);
+
+extern int __init efi_memmap_install(struct efi_memory_map_data *data);
+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);
+
 #define arch_ima_efi_boot_mode \
({ extern struct boot_params boot_params; boot_params.secure_boot; })
 
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index b481719b16cc..ed5502a5185d 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,6 +2,7 @@
 KASAN_SANITIZE := n
 GCOV_PROFILE := n
 
-obj-$(CONFIG_EFI)  += quirks.o efi.o efi_$(BITS).o 
efi_stub_$(BITS).o
+obj-$(CONFIG_EFI)  += memmap.o quirks.o efi.o efi_$(BITS).o \
+  efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)+= efi_thunk_$(BITS).o
 obj-$(CONFIG_EFI_FAKE_MEMMAP)  += fake_mem.o
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
new file mode 100644
index ..44b886acf301
--- /dev/null
+++ b/arch/x86/platform/efi/memmap.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common EFI memory map functions.
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
+{
+   return memblock_phys_alloc(size, SMP_CACHE_BYTES);
+}
+
+static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
+{
+   unsigned int order = get_order(size);
+   struct page *p = alloc_pages(GFP_KERNEL, order);
+
+   if (!p)
+   return 0;
+
+   return PFN_PHYS(page_to_pfn(p));
+}
+
+void __init __efi_memmap_free(u64 phys, unsigned long size, unsigned long 
flags)
+{
+   if (flags & EFI_MEMMAP_MEMBLOCK) {
+   if (slab_is_available())
+   memblock_free_late(phys, size);
+   else
+   memblock_phys_free(phys, size);
+   } else if (flags & EFI_MEMMAP_SLAB) {
+   struct page *p = pfn_to_page(PHYS_PFN(phys));
+   unsigned int order = get_order(size);
+
+   free_pages((unsigned long) page_address(p), order);
+   }
+}
+
+/**
+ * efi_memmap_alloc - Allocate memory for the EFI memory map
+ * @num_entries: Number of entries in the allocated map.
+ * @data: efi memmap installation parameters
+ *
+ * Depending on whether mm_init() has already been invoked or not,
+ * either memblock or "normal" page allocation is used.
+ *
+ * Returns zero on success, a negative error code on failure.
+ */
+int __init efi_memmap_alloc(unsigned int num_entries,
+   struct efi_memory_map_data *data)
+{
+   /* Expect allocation parameters are zero initialized */
+   WARN_ON(data->phys_map || data->size);
+
+   data->size = num_entries * efi.memmap.desc_size;
+   data->desc_version = efi.memmap.desc_version;
+   data->desc_size = efi.memmap.desc_size;
+   data->flags &= ~(EFI_MEMMAP_SLAB | EFI_MEMMAP_MEMBLOCK);
+   data->flags |= efi.memmap.flags & EFI_MEMMAP_LATE;
+
+   if (slab_is_available()) {
+   data->flags |= EFI_MEMMAP_SLAB;
+   data->phys_map = __efi_memmap_alloc_late(data->size);
+   } else {
+   data->flags |= EFI_MEMMAP_MEMBLOCK;
+   data->phys_map = __efi_memmap_alloc_early(data->size);
+   }
+
+   if (!data->phys_map)
+   return -ENOMEM;
+   return 0;
+}
+
+/**
+ * efi_memmap_install - Install a new EFI memory map in efi.memmap
+ * @ctx: map allocation parameters (address, size, flags)
+ *
+ * Unlike efi_memmap_init_*(), thi

[RFC PATCH 3/5] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures

[Ard Biesheuvel]

Currently, the EFI_PARAVIRT flag is only used by x86, even though other
architectures also support pseudo-EFI boot, where the core kernel is
invoked directly and provided with a set of data tables that resemble
the ones constructed by the EFI stub, which never actually runs in that
case.

Let's fix this inconsistency, and always set this flag when booting dom0
via the EFI boot path. Note that Xen on x86 does not provide the EFI
memory map in this case, whereas other architectures do, so move the
associated EFI_PARAVIRT check into the x86 platform code.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/platform/efi/efi.c  | 8 +---
 arch/x86/platform/efi/memmap.c   | 3 +++
 drivers/firmware/efi/fdtparams.c | 4 
 drivers/firmware/efi/memmap.c| 3 ---
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 6e598bd78eef..6a6f2a585a3d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -214,9 +214,11 @@ int __init efi_memblock_x86_reserve_range(void)
data.desc_size  = e->efi_memdesc_size;
data.desc_version   = e->efi_memdesc_version;
 
-   rv = efi_memmap_init_early();
-   if (rv)
-   return rv;
+   if (!efi_enabled(EFI_PARAVIRT)) {
+   rv = efi_memmap_init_early();
+   if (rv)
+   return rv;
+   }
 
if (add_efi_memmap || do_efi_soft_reserve())
do_add_efi_memmap();
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
index 44b886acf301..18e14ec16720 100644
--- a/arch/x86/platform/efi/memmap.c
+++ b/arch/x86/platform/efi/memmap.c
@@ -93,6 +93,9 @@ int __init efi_memmap_install(struct efi_memory_map_data 
*data)
 {
efi_memmap_unmap();
 
+   if (efi_enabled(EFI_PARAVIRT))
+   return 0;
+
return __efi_memmap_init(data);
 }
 
diff --git a/drivers/firmware/efi/fdtparams.c b/drivers/firmware/efi/fdtparams.c
index e901f8564ca0..0ec83ba58097 100644
--- a/drivers/firmware/efi/fdtparams.c
+++ b/drivers/firmware/efi/fdtparams.c
@@ -30,11 +30,13 @@ static __initconst const char name[][22] = {
 
 static __initconst const struct {
const char  path[17];
+   u8  paravirt;
const char  params[PARAMCOUNT][26];
 } dt_params[] = {
{
 #ifdef CONFIG_XEN//  <---17-->
.path = "/hypervisor/uefi",
+   .paravirt = 1,
.params = {
[SYSTAB] = "xen,uefi-system-table",
[MMBASE] = "xen,uefi-mmap-start",
@@ -121,6 +123,8 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data 
*mm)
pr_err("Can't find property '%s' in DT!\n", pname);
return 0;
}
+   if (dt_params[i].paravirt)
+   set_bit(EFI_PARAVIRT, );
return systab;
}
 notfound:
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 3501d3814f22..9508082af907 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -44,9 +44,6 @@ int __init __efi_memmap_init(struct efi_memory_map_data *data)
struct efi_memory_map map;
phys_addr_t phys_map;
 
-   if (efi_enabled(EFI_PARAVIRT))
-   return 0;
-
phys_map = data->phys_map;
 
if (data->flags & EFI_MEMMAP_LATE)
-- 
2.35.1

[RFC PATCH 1/5] efi: Move EFI fake memmap support into x86 arch tree

[Ard Biesheuvel]

The EFI fake memmap support is specific to x86, which manipulates the
EFI memory map in various different ways after receiving it from the EFI
stub. On other architectures, we have manages to push back on this, and
the EFI memory map is kept pristine.

So let's move the fake memmap code into the x86 arch tree, where it
arguably belongs.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/Kconfig   | 20 +
 arch/x86/include/asm/efi.h |  5 ++
 arch/x86/kernel/setup.c|  1 +
 arch/x86/platform/efi/Makefile |  1 +
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c | 79 
+++-
 drivers/firmware/efi/Kconfig   | 22 --
 drivers/firmware/efi/Makefile  |  4 -
 drivers/firmware/efi/fake_mem.h| 10 ---
 drivers/firmware/efi/x86_fake_mem.c| 75 ---
 include/linux/efi.h|  6 --
 10 files changed, 103 insertions(+), 120 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..b98941c2fec4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1978,6 +1978,26 @@ config EFI_MIXED
 
  If unsure, say N.
 
+config EFI_FAKE_MEMMAP
+   bool "Enable EFI fake memory map"
+   depends on EFI
+   help
+ Saying Y here will enable "efi_fake_mem" boot option.  By specifying
+ this parameter, you can add arbitrary attribute to specific memory
+ range by updating original (firmware provided) EFI memmap.  This is
+ useful for debugging of EFI memmap related feature, e.g., Address
+ Range Mirroring feature.
+
+config EFI_MAX_FAKE_MEM
+   int "maximum allowable number of ranges in efi_fake_mem boot option"
+   depends on EFI_FAKE_MEMMAP
+   range 1 128
+   default 8
+   help
+ Maximum allowable number of ranges in efi_fake_mem boot option.
+ Ranges can be set up to this value using comma-separated list.
+ The default value is 8.
+
 source "kernel/Kconfig.hz"
 
 config KEXEC
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 897ea4aec16e..68414d924332 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -404,10 +404,15 @@ static inline void efi_reserve_boot_services(void)
 
 #ifdef CONFIG_EFI_FAKE_MEMMAP
 extern void __init efi_fake_memmap_early(void);
+extern void __init efi_fake_memmap(void);
 #else
 static inline void efi_fake_memmap_early(void)
 {
 }
+
+static inline void efi_fake_memmap(void)
+{
+}
 #endif
 
 #define arch_ima_efi_boot_mode \
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7144ee..41ec3a69f3c7 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -31,6 +31,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index a50245157685..b481719b16cc 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -4,3 +4,4 @@ GCOV_PROFILE := n
 
 obj-$(CONFIG_EFI)  += quirks.o efi.o efi_$(BITS).o 
efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_FAKE_MEMMAP)  += fake_mem.o
diff --git a/drivers/firmware/efi/fake_mem.c b/arch/x86/platform/efi/fake_mem.c
similarity index 58%
rename from drivers/firmware/efi/fake_mem.c
rename to arch/x86/platform/efi/fake_mem.c
index 6e0f34a38171..41d57cad3d84 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/arch/x86/platform/efi/fake_mem.c
@@ -17,10 +17,13 @@
 #include 
 #include 
 #include 
-#include "fake_mem.h"
+#include 
+#include 
 
-struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
-int nr_fake_mem;
+#define EFI_MAX_FAKEMEM CONFIG_EFI_MAX_FAKE_MEM
+
+static struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
+static int nr_fake_mem;
 
 static int __init cmp_fake_mem(const void *x1, const void *x2)
 {
@@ -122,3 +125,73 @@ static int __init setup_fake_mem(char *p)
 }
 
 early_param("efi_fake_mem", setup_fake_mem);
+
+void __init efi_fake_memmap_early(void)
+{
+   int i;
+
+   /*
+* The late efi_fake_mem() call can handle all requests if
+* EFI_MEMORY_SP support is disabled.
+*/
+   if (!efi_soft_reserve_enabled())
+   return;
+
+   if (!efi_enabled(EFI_MEMMAP) || !nr_fake_mem)
+   return;
+
+   /*
+* Given that efi_fake_memmap() needs to perform memblock
+* allocations it needs to run after e820__memblock_setup().
+* However, if efi_fake_mem specifies EFI_MEMORY_SP for a given
+* address range that potentially needs to mark the memory as
+* reserved prior to e820__memblock_setup(). Update e820
+* directly if EFI_MEMORY_SP is

[RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen

[Ard Biesheuvel]

This is an alternate approach to addressing the issue that Demi Marie is
attempting to fix in [0] (i.e., ESRT config table exposed to a x86 dom0
is corrupted because it resides in boot services memory as per the EFI
spec, where it gets corrupted by Xen). My main objection to that approach
is that it needs Xen-specific fixes in multiple different places, but we
still end up only fixing the ESRT case specifically.

So instead, I am proposing this series as a more generic way to handle
configuration tables that reside in boot services memory, and confining
the Xen specific logic to the Xen EFI glue code.

Given that EFI boot without a memory map is only permitted on x86 and
only when doing Xen boot, let's clear up some inconsistencies there
first so we can set the EFI_PARAVIRT flag on all architectures that do
pseudo-EFI boot straight into the core kernel (i.e., without going
through the stub). This moves a good chunk of EFI memory map
manipulation code into the x86 arch tree, where it arguably belongs as
no other architectures rely on it. This is implemented in patches 1 - 3.

Patch #4 updates the configuration table iteration logic so that only
ACPI and SMBIOS tables are exposed automatically when EFI_PARAVIRT is
set, and other config tables only if they reside in a memory region of a
EFI memory type that is guaranteed to be preserved. This effectively
hides the ESRT, but also the memory attributes table and the runtime
properties (and potentially others) when doing Xen dom0 boot unless they
have been moved out of EFI boot services memory.

The final patch relaxes the ESRT sanity check so that the ESRT is parsed
and exposed even if EFI_MEMMAP is not set, which is the case with Xen
dom0 on x86. If additional memory map checks are required in this code
path, the best way to achieve this is for Xen to expose a EFI memory map
on x86 just like it does on other architectures that support Xen (ARM
and arm64)

[0] 
https://lore.kernel.org/all/cover.1664298147.git.d...@invisiblethingslab.com/

Cc: Demi Marie Obenour 
Cc: Peter Jones 
Cc: Juergen Gross 
Cc: Stefano Stabellini 
Cc: Oleksandr Tyshchenko 
Cc: Kees Cook 
Cc: Anton Vorontsov 
Cc: Colin Cross 
Cc: Tony Luck 
Cc: Marek Marczykowski-Górecki 

Ard Biesheuvel (5):
  efi: Move EFI fake memmap support into x86 arch tree
  efi: memmap: Move manipulation routines into x86 arch tree
  efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures
  efi: Apply allowlist to EFI configuration tables when running under
Xen
  efi: esrt: Omit region sanity check when no memory map is available

 arch/x86/Kconfig   |  20 ++
 arch/x86/include/asm/efi.h |  16 ++
 arch/x86/kernel/setup.c|   1 +
 arch/x86/platform/efi/Makefile |   4 +-
 arch/x86/platform/efi/efi.c|   8 +-
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c |  79 ++-
 arch/x86/platform/efi/memmap.c | 238 

 arch/x86/platform/efi/quirks.c |   3 +
 drivers/firmware/efi/Kconfig   |  22 --
 drivers/firmware/efi/Makefile  |   4 -
 drivers/firmware/efi/efi.c |   7 +
 drivers/firmware/efi/esrt.c|  61 ++---
 drivers/firmware/efi/fake_mem.h|  10 -
 drivers/firmware/efi/fdtparams.c   |   4 +
 drivers/firmware/efi/memmap.c  | 224 +-
 drivers/firmware/efi/x86_fake_mem.c|  75 --
 drivers/xen/efi.c  |  69 ++
 include/linux/efi.h|  18 +-
 18 files changed, 481 insertions(+), 382 deletions(-)
 rename {drivers/firmware => arch/x86/platform}/efi/fake_mem.c (58%)
 create mode 100644 arch/x86/platform/efi/memmap.c
 delete mode 100644 drivers/firmware/efi/fake_mem.h
 delete mode 100644 drivers/firmware/efi/x86_fake_mem.c

-- 
2.35.1

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 22:59, Ard Biesheuvel  wrote:
>
> On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > >  wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > >  wrote:
> > > > > >
> > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > discover which firmware can be updated by the OS.  Currently, Linux 
> > > > > > does
> > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is 
> > > > > > not
> > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for 
> > > > > > e.g.
> > > > > > Qubes OS.
> > > > > >
> > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > The UEFI specification requires the ESRT to be in 
> > > > > > EfiBootServicesData
> > > > > > memory, which Xen will use for whatever purposes it likes.  
> > > > > > Therefore,
> > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > >
> > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > reserved.  Such memory is currently of type 
> > > > > > EFI_RUNTIME_SERVICES_DATA,
> > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > >
> > > > > > When running as a Xen dom0, use the new
> > > > > > xen_config_table_memory_region_max() function to determine if Xen 
> > > > > > has
> > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > > possible.
> > > > > >
> > > > > > Signed-off-by: Demi Marie Obenour 
> > > > >
> > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > false when patch 1/2 is applied.
> > > >
> > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > alternative way to get the end of the memory region containing the ESRT.
> > > > That is what this patch provides.
> > >
> > > OK. I don't think we need that to be honest. When running under Xen,
> > > we should be able to assume that the ESRT does not span multiple
> > > memory regions arbitrarily, so we can just omit this check if
> > > !efi_enabled(EFI_MEMMAP)
> > >
> > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > don't think we need that for Xen.
> >
> > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > under Xen than on bare hardware.
>
> That may be true. But if Xen needs dom0 to be able to cross reference
> the EFI memory map, it should provide one (and set EFI_MEMMAP to
> enabled).

Btw the efi_mem_reserve() for the ESRT is also redundant if it is
guaranteed to be in RT services data or ACPI reclaim memory.

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> >  wrote:
> > >
> > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > discover which firmware can be updated by the OS.  Currently, Linux 
> > > > > does
> > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > possible to use fwupd in a Xen dom0, which is a serious problem for 
> > > > > e.g.
> > > > > Qubes OS.
> > > > >
> > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > >
> > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > EfiBootServicesData
> > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > EfiBootServicesData
> > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > >
> > > > > When running as a Xen dom0, use the new
> > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > containing it.  This allows programs such as fwupd which require the
> > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > possible.
> > > > >
> > > > > Signed-off-by: Demi Marie Obenour 
> > > >
> > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > false when patch 1/2 is applied.
> > >
> > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > alternative way to get the end of the memory region containing the ESRT.
> > > That is what this patch provides.
> >
> > OK. I don't think we need that to be honest. When running under Xen,
> > we should be able to assume that the ESRT does not span multiple
> > memory regions arbitrarily, so we can just omit this check if
> > !efi_enabled(EFI_MEMMAP)
> >
> > IIRC (and Peter would know), we are trying to filter out descriptors
> > that are completely bogus here: zero lenght, zero address, etc etc. I
> > don't think we need that for Xen.
>
> Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> under Xen than on bare hardware.

That may be true. But if Xen needs dom0 to be able to cross reference
the EFI memory map, it should provide one (and set EFI_MEMMAP to
enabled).

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> >  wrote:
> > >
> > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > Qubes OS.
> > >
> > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > >
> > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > ensures that the ESRT can safely be accessed by the OS.
> > >
> > > When running as a Xen dom0, use the new
> > > xen_config_table_memory_region_max() function to determine if Xen has
> > > reserved the ESRT and, if so, find the end of the memory region
> > > containing it.  This allows programs such as fwupd which require the
> > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > >
> > > Signed-off-by: Demi Marie Obenour 
> >
> > Why do we need this patch? I'd expect esrt_table_exists() to return
> > false when patch 1/2 is applied.
>
> efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> alternative way to get the end of the memory region containing the ESRT.
> That is what this patch provides.

OK. I don't think we need that to be honest. When running under Xen,
we should be able to assume that the ESRT does not span multiple
memory regions arbitrarily, so we can just omit this check if
!efi_enabled(EFI_MEMMAP)

IIRC (and Peter would know), we are trying to filter out descriptors
that are completely bogus here: zero lenght, zero address, etc etc. I
don't think we need that for Xen.

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 20:17, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:25:53PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> >  wrote:
> > >
> > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > memory types are not suitable for EFI tables, leaving only
> > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > use tables that are located in one of these types of memory.
> > >
> > > This patch ensures this, and also adds a function
> > > (xen_config_table_memory_region_max()) that will be used later to
> > > replace the usage of the EFI memory map in esrt.c when running under
> > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > but I have not implemented this.
> > >
> > > Signed-off-by: Demi Marie Obenour 
> > > ---
> > >  drivers/firmware/efi/efi.c |  8 +---
> > >  drivers/xen/efi.c  | 35 +++
> > >  include/linux/efi.h|  9 +
> > >  3 files changed, 49 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 
> > > e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
> > >  100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > > unsigned long table;
> > > int i;
> > >
> > > -   pr_info("");
> >
> > Why are you removing these prints?
>
> If I left them, I would need to include a pr_cont("\n") later.

There should always be one at the end of the loop, no? Or is this
related to the diagnostic that gets printed in your helper?

> Checkpatch recommends against that.  What is the purpose of this print?
> I assumed that since it prints an empty string it is superfluous.
>

It prints the leading [invisible] loglevel marker, and the 'efi: ' prefix.

> > > for (i = 0; i < count; i++) {
> > > if (!IS_ENABLED(CONFIG_X86)) {
> > > guid = _tables[i].guid;
> > > @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > >
> > > if (IS_ENABLED(CONFIG_X86_32) &&
> > > tbl64[i].table > U32_MAX) {
> > > -   pr_cont("\n");
> > > pr_err("Table located above 4GB, 
> > > disabling EFI.\n");
> > > return -EINVAL;
> > > }
> > > @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > > table = tbl32[i].table;
> > > }
> > >
> > > +#ifdef CONFIG_XEN_EFI
> >
> > We tend to prefer IS_ENABLED() for cases such as this one. That way,
> > the compiler always gets to see the code inside the conditional block,
> > which gives better build test coverage (even if CONFIG_XEN_EFI is
> > disabled).
>
> Can I count on the compiler eliminating the code as unreachable?  With
> CONFIG_XEN_EFI disabled xen_config_table_memory_region_max() would be an
> undefined symbol.
>

If you drop the #ifdef in the .h file (as I suggested below) the code
will compile fine, and the symbol reference will not be emitted into
the object, so it will link fine even if the Xen objects are not being
built.

We rely on this behavior all over the Linux kernel.

> > > +   if (efi_enabled(EFI_PARAVIRT) && 
> > > !xen_config_table_memory_region_max(table))
> >
> > So the question here is whether Xen thinks the table should be
> > disregarded or not. So let's define a prototype that reflects that
> > purpose, and let the implementation reason about how this should be
> > achieved.
>
> xen_config_table_memory_region_max() doesn’t just return whether the
> table should be disregard

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 19:12, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:30:57PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
> > >
> > > On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, 
> > > > EFI_LOADER_DATA,
> > > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > > memory types are not suitable for EFI tables, leaving only
> > > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > > use tables that are located in one of these types of memory.
> > > >
> > > > This patch ensures this, and also adds a function
> > > > (xen_config_table_memory_region_max()) that will be used later to
> > > > replace the usage of the EFI memory map in esrt.c when running under
> > > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > > but I have not implemented this.
> > > >
> > > > Signed-off-by: Demi Marie Obenour 
> > >
> > > In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> > > "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> > > also use tables located in such regions, perhaps by faking
> > > EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
> > >
> >
> > I know this ship has sailed for x86, but for the sake of other
> > architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
> > bits alone, for the same reasons I gave earlier. (Runtime mappings for
> > the firmware code itself, page table fragmentation etc etc)
>
> Why do you say that it has sailed for x86?
>

The x86 EFI code in Linux makes changes to the EFI memory map in many
different places in the code. On other architectures, we have managed
to avoid this, so that the EFI memory map is always identical to the
one provided by the firmware at boot.

> > I know very little about Xen, but based on the context you provided in
> > this thread, I'd say that the best approach from the Xen side is to
> > convert all EfiBootServicesData regions that have configuration tables
> > pointing into them into EfiAcpiReclaimMemory.
>
> Should Xen convert the entire region, or should it try to reserve only
> the memory it needs?  The latter would require it to parse the
> configuration tables.  Is there a list of configuration tables that can
> legitimately be in EfiBootServicesData regions?
>

Not really, no. So you would have to convert the entire region
/unless/ Xen knows the GUID, and therefore knows how to derive the
size of the table, allowing it to reserve memory more conservatively.
However, I doubt whether this is worth it: splitting entries implies
rewriting the memory map, which is a thing I'd rather avoid if I were
in your shoes.

> > I take it XEN_FW_EFI_MEM_INFO is an existing interface? If so, you
> > might do the same for the returned type - EfiBootServicesData ->
> > EfiAcpiReclaimMemory, and not muck about with the EFI_MEMORY_RUNTIME
> > attribute.
>
> It is indeed an existing interface, and this is a much better idea than
> what you proposed.

Right.

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
 wrote:
>
> fwupd requires access to the EFI System Resource Table (ESRT) to
> discover which firmware can be updated by the OS.  Currently, Linux does
> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> Qubes OS.
>
> Before Xen 4.17, this was not fixable due to hypervisor limitations.
> The UEFI specification requires the ESRT to be in EfiBootServicesData
> memory, which Xen will use for whatever purposes it likes.  Therefore,
> Linux cannot safely access the ESRT, as Xen may have overwritten it.
>
> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> memory, Xen replaces the ESRT with a copy in memory that it has
> reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> ensures that the ESRT can safely be accessed by the OS.
>
> When running as a Xen dom0, use the new
> xen_config_table_memory_region_max() function to determine if Xen has
> reserved the ESRT and, if so, find the end of the memory region
> containing it.  This allows programs such as fwupd which require the
> ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
>
> Signed-off-by: Demi Marie Obenour 

Why do we need this patch? I'd expect esrt_table_exists() to return
false when patch 1/2 is applied.



> ---
>  drivers/firmware/efi/esrt.c | 43 ++---
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 
> 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..a0642bc161b4b1f94f818b8c9f46511fe2424bb2
>  100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -243,27 +243,44 @@ void __init efi_esrt_init(void)
> void *va;
> struct efi_system_resource_table tmpesrt;
> size_t size, max, entry_size, entries_size;
> -   efi_memory_desc_t md;
> -   int rc;
> phys_addr_t end;
> -
> -   if (!efi_enabled(EFI_MEMMAP))
> -   return;
> +   u32 type;
>
> pr_debug("esrt-init: loading.\n");
> if (!esrt_table_exists())
> return;
>
> -   rc = efi_mem_desc_lookup(efi.esrt, );
> -   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");
> +   if (efi_enabled(EFI_MEMMAP)) {
> +   efi_memory_desc_t md;
> +
> +   if (efi_mem_desc_lookup(efi.esrt, ) < 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;
> +   }
> +
> +   type = md.type;
> +   max = efi_mem_desc_end();
> +#ifdef CONFIG_XEN_EFI
> +   } else if (efi_enabled(EFI_PARAVIRT)) {
> +   max = xen_config_table_memory_region_max(efi.esrt);
> +   /*
> +* This might be wrong, but it doesn't matter.
> +* xen_config_table_memory_region_max() checks the type
> +* of the memory region, and if it returns 0, the code
> +* below will fail without looking at the type.  Choose
> +* a value that will not cause * subsequent code to try
> +* to reserve the memory containing the ESRT, as either
> +* Xen or the firmware has done so already.
> +*/
> +   type = EFI_RUNTIME_SERVICES_DATA;
> +#endif
> +   } else {
> return;
> }
>
> -   max = efi_mem_desc_end();
> if (max < efi.esrt) {
> pr_err("EFI memory descriptor is invalid. (esrt: %p max: 
> %p)\n",
>(void *)efi.esrt, (void *)max);
> @@ -333,7 +350,7 @@ void __init efi_esrt_init(void)
>
> end = esrt_data + size;
> pr_info("Reserving ESRT space from %pa to %pa.\n", _data, );
> -   if (md.type == EFI_BOOT_SERVICES_DATA)
> +   if (type == EFI_BOOT_SERVICES_DATA)
> efi_mem_reserve(esrt_data, esrt_data_size);
>
> pr_debug("esrt-init: loaded.\n");
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
>

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
>
> On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > must not use EFI tables from such memory.  Most of the remaining EFI
> > memory types are not suitable for EFI tables, leaving only
> > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > use tables that are located in one of these types of memory.
> >
> > This patch ensures this, and also adds a function
> > (xen_config_table_memory_region_max()) that will be used later to
> > replace the usage of the EFI memory map in esrt.c when running under
> > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > but I have not implemented this.
> >
> > Signed-off-by: Demi Marie Obenour 
>
> In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> also use tables located in such regions, perhaps by faking
> EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
>

I know this ship has sailed for x86, but for the sake of other
architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
bits alone, for the same reasons I gave earlier. (Runtime mappings for
the firmware code itself, page table fragmentation etc etc)

I know very little about Xen, but based on the context you provided in
this thread, I'd say that the best approach from the Xen side is to
convert all EfiBootServicesData regions that have configuration tables
pointing into them into EfiAcpiReclaimMemory.

I take it XEN_FW_EFI_MEM_INFO is an existing interface? If so, you
might do the same for the returned type - EfiBootServicesData ->
EfiAcpiReclaimMemory, and not muck about with the EFI_MEMORY_RUNTIME
attribute.

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

[Ard Biesheuvel]

On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
 wrote:
>
> Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> Xen before Linux gets to start using it.  Therefore, Linux under Xen
> must not use EFI tables from such memory.  Most of the remaining EFI
> memory types are not suitable for EFI tables, leaving only
> EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> use tables that are located in one of these types of memory.
>
> This patch ensures this, and also adds a function
> (xen_config_table_memory_region_max()) that will be used later to
> replace the usage of the EFI memory map in esrt.c when running under
> Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> but I have not implemented this.
>
> Signed-off-by: Demi Marie Obenour 
> ---
>  drivers/firmware/efi/efi.c |  8 +---
>  drivers/xen/efi.c  | 35 +++
>  include/linux/efi.h|  9 +
>  3 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 
> e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
>  100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
> unsigned long table;
> int i;
>
> -   pr_info("");

Why are you removing these prints?

> for (i = 0; i < count; i++) {
> if (!IS_ENABLED(CONFIG_X86)) {
> guid = _tables[i].guid;
> @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
>
> if (IS_ENABLED(CONFIG_X86_32) &&
> tbl64[i].table > U32_MAX) {
> -   pr_cont("\n");
> pr_err("Table located above 4GB, disabling 
> EFI.\n");
> return -EINVAL;
> }
> @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
> table = tbl32[i].table;
> }
>
> +#ifdef CONFIG_XEN_EFI

We tend to prefer IS_ENABLED() for cases such as this one. That way,
the compiler always gets to see the code inside the conditional block,
which gives better build test coverage (even if CONFIG_XEN_EFI is
disabled).

> +   if (efi_enabled(EFI_PARAVIRT) && 
> !xen_config_table_memory_region_max(table))

So the question here is whether Xen thinks the table should be
disregarded or not. So let's define a prototype that reflects that
purpose, and let the implementation reason about how this should be
achieved.

So

if (IS_ENABLED(CONFIG_XEN_EFI) &&
efi_enabled(EFI_PARAVIRT) &&
xen_efi_config_table_valid(guid, table)
continue

I should note here, though, that EFI_PARAViRT is only set on x86 not
on other architectures that enable CONFIG_XEN_EFI so this will not
work anywhere else.


> +   continue;
> +#endif
> +
> if (!match_config_table(guid, table, common_tables) && 
> arch_tables)
> match_config_table(guid, table, arch_tables);
> }
> -   pr_cont("\n");
> set_bit(EFI_CONFIG_TABLES, );
>
> if (efi_rng_seed != EFI_INVALID_TABLE_ADDR) {
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index 
> d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..c2274ddfcc63304008ef0fd78fd9fa416f75d073
>  100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -271,6 +272,40 @@ static void xen_efi_reset_system(int reset_type, 
> efi_status_t status,
> }
>  }
>
> +__init u64 xen_config_table_memory_region_max(u64 addr)

It is more idiomatic for Linux to put __init after the return type.
And if we adopt my suggestion above, this becomes

bool __init xen_efi_config_table_valid(const efi_guid_t *guid, u64 table)

Alternatively, you could pass the string identifier of the table
instead of the guid (or both) to print in the diagnostic message.


> +{
> +   static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");

Is this the only place where this matters? And this never happens on x86, right?

> +   struct xen_platform_op op = {
> +   .cmd = XENPF_firmware_info,
> +   .u.firmware_info = {
> +   .type = XEN_FW_EFI_INFO,
> +   .index = XEN_FW_EFI_MEM_INFO,
> +   .u.efi_info.mem.addr = addr,
> +   .u.efi_info.mem.size = U64_MAX - addr,
> +   }
> +   };
> +   union 

Re: [PATCH v3] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Fri, 23 Sept 2022 at 01:27, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 23, 2022 at 12:14:50AM +0200, Ard Biesheuvel wrote:
> > On Thu, 22 Sept 2022 at 20:12, Demi Marie Obenour
> >  wrote:
> > >
> > > On Thu, Sep 22, 2022 at 05:05:43PM +0200, Ard Biesheuvel wrote:
> > > > On Thu, 22 Sept 2022 at 16:56, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > On Thu, Sep 22, 2022 at 08:12:14AM +0200, Jan Beulich wrote:
> > > > > > On 22.09.2022 03:09, Demi Marie Obenour wrote:
> > > > > > > On Wed, Sep 21, 2022 at 10:34:04PM +0200, Jan Beulich wrote:
> > > > > > >> On 20.09.2022 18:09, Ard Biesheuvel wrote:
> > > > > > >>> On Tue, 20 Sept 2022 at 17:54, Jan Beulich  
> > > > > > >>> wrote:
> > > > > > >>>>
> > > > > > >>>> On 20.09.2022 17:36, Ard Biesheuvel wrote:
> > > > > > >>>>> On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
> > > > > > >>>>>  wrote:
> > > > > > >>>>>>
> > > > > > >>>>>> fwupd requires access to the EFI System Resource Table 
> > > > > > >>>>>> (ESRT) to
> > > > > > >>>>>> discover which firmware can be updated by the OS.  
> > > > > > >>>>>> Currently, Linux does
> > > > > > >>>>>> not expose the ESRT when running as a Xen dom0.  Therefore, 
> > > > > > >>>>>> it is not
> > > > > > >>>>>> possible to use fwupd in a Xen dom0, which is a serious 
> > > > > > >>>>>> problem for e.g.
> > > > > > >>>>>> Qubes OS.
> > > > > > >>>>>>
> > > > > > >>>>>> Before Xen 4.16, this was not fixable due to hypervisor 
> > > > > > >>>>>> limitations.
> > > > > > >>>>>> The UEFI specification requires the ESRT to be in 
> > > > > > >>>>>> EfiBootServicesData
> > > > > > >>>>>> memory, which Xen will use for whatever purposes it likes.  
> > > > > > >>>>>> Therefore,
> > > > > > >>>>>> Linux cannot safely access the ESRT, as Xen may have 
> > > > > > >>>>>> overwritten it.
> > > > > > >>>>>>
> > > > > > >>>>>> Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > > >>>>>> EfiBootServicesData
> > > > > > >>>>>> or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > > >>>>>> EfiBootServicesData
> > > > > > >>>>>> memory, Xen allocates some memory of type 
> > > > > > >>>>>> EfiRuntimeServicesData, copies
> > > > > > >>>>>> the ESRT to it, and finally replaces the ESRT pointer with a 
> > > > > > >>>>>> pointer to
> > > > > > >>>>>> the copy.  Since Xen will not clobber EfiRuntimeServicesData 
> > > > > > >>>>>> memory,
> > > > > > >>>>>> this ensures that the ESRT can safely be accessed by the OS. 
> > > > > > >>>>>>  It is safe
> > > > > > >>>>>> to access the ESRT under Xen if, and only if, it is in 
> > > > > > >>>>>> memory of type
> > > > > > >>>>>> EfiRuntimeServicesData.
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>> Thanks for the elaborate explanation. This is really helpful.
> > > > > > >>>>>
> > > > > > >>>>> So here, you are explaining that the only way for Xen to 
> > > > > > >>>>> prevent
> > > > > > >>>>> itself from potentially clobbering the ESRT is by creating a
> > > > > > >>>>> completely new allocation?
> > > > > > >>>>
> > > > > > >>>> There are surely other ways, e.

Re: [PATCH v3] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Thu, 22 Sept 2022 at 20:12, Demi Marie Obenour
 wrote:
>
> On Thu, Sep 22, 2022 at 05:05:43PM +0200, Ard Biesheuvel wrote:
> > On Thu, 22 Sept 2022 at 16:56, Demi Marie Obenour
> >  wrote:
> > >
> > > On Thu, Sep 22, 2022 at 08:12:14AM +0200, Jan Beulich wrote:
> > > > On 22.09.2022 03:09, Demi Marie Obenour wrote:
> > > > > On Wed, Sep 21, 2022 at 10:34:04PM +0200, Jan Beulich wrote:
> > > > >> On 20.09.2022 18:09, Ard Biesheuvel wrote:
> > > > >>> On Tue, 20 Sept 2022 at 17:54, Jan Beulich  
> > > > >>> wrote:
> > > > >>>>
> > > > >>>> On 20.09.2022 17:36, Ard Biesheuvel wrote:
> > > > >>>>> On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
> > > > >>>>>  wrote:
> > > > >>>>>>
> > > > >>>>>> fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > >>>>>> discover which firmware can be updated by the OS.  Currently, 
> > > > >>>>>> Linux does
> > > > >>>>>> not expose the ESRT when running as a Xen dom0.  Therefore, it 
> > > > >>>>>> is not
> > > > >>>>>> possible to use fwupd in a Xen dom0, which is a serious problem 
> > > > >>>>>> for e.g.
> > > > >>>>>> Qubes OS.
> > > > >>>>>>
> > > > >>>>>> Before Xen 4.16, this was not fixable due to hypervisor 
> > > > >>>>>> limitations.
> > > > >>>>>> The UEFI specification requires the ESRT to be in 
> > > > >>>>>> EfiBootServicesData
> > > > >>>>>> memory, which Xen will use for whatever purposes it likes.  
> > > > >>>>>> Therefore,
> > > > >>>>>> Linux cannot safely access the ESRT, as Xen may have overwritten 
> > > > >>>>>> it.
> > > > >>>>>>
> > > > >>>>>> Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > >>>>>> EfiBootServicesData
> > > > >>>>>> or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > >>>>>> EfiBootServicesData
> > > > >>>>>> memory, Xen allocates some memory of type 
> > > > >>>>>> EfiRuntimeServicesData, copies
> > > > >>>>>> the ESRT to it, and finally replaces the ESRT pointer with a 
> > > > >>>>>> pointer to
> > > > >>>>>> the copy.  Since Xen will not clobber EfiRuntimeServicesData 
> > > > >>>>>> memory,
> > > > >>>>>> this ensures that the ESRT can safely be accessed by the OS.  It 
> > > > >>>>>> is safe
> > > > >>>>>> to access the ESRT under Xen if, and only if, it is in memory of 
> > > > >>>>>> type
> > > > >>>>>> EfiRuntimeServicesData.
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>> Thanks for the elaborate explanation. This is really helpful.
> > > > >>>>>
> > > > >>>>> So here, you are explaining that the only way for Xen to prevent
> > > > >>>>> itself from potentially clobbering the ESRT is by creating a
> > > > >>>>> completely new allocation?
> > > > >>>>
> > > > >>>> There are surely other ways, e.g. preserving BootServices* regions
> > > > >>>> alongside RuntimeServices* ones. But as the maintainer of the EFI
> > > > >>>> code in Xen I don't view this as a reasonable approach.
> > > > >>>
> > > > >>> Why not?
> > > > >>
> > > > >> Because it's against the intentions the EFI has (or at least had)
> > > > >> for this memory type. Much more than EfiAcpiReclaimMemory this
> > > > >> type is intended for use as ordinary RAM post-boot.
> > > > >
> > > > > What about giving that memory to dom0?  dom0’s balloon driver will 
> > > > > give
> > > > > anything dom0 doesn’t wind up using back to Xen.
> > > 

Re: [PATCH v3] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Thu, 22 Sept 2022 at 16:56, Demi Marie Obenour
 wrote:
>
> On Thu, Sep 22, 2022 at 08:12:14AM +0200, Jan Beulich wrote:
> > On 22.09.2022 03:09, Demi Marie Obenour wrote:
> > > On Wed, Sep 21, 2022 at 10:34:04PM +0200, Jan Beulich wrote:
> > >> On 20.09.2022 18:09, Ard Biesheuvel wrote:
> > >>> On Tue, 20 Sept 2022 at 17:54, Jan Beulich  wrote:
> > >>>>
> > >>>> On 20.09.2022 17:36, Ard Biesheuvel wrote:
> > >>>>> On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
> > >>>>>  wrote:
> > >>>>>>
> > >>>>>> fwupd requires access to the EFI System Resource Table (ESRT) to
> > >>>>>> discover which firmware can be updated by the OS.  Currently, Linux 
> > >>>>>> does
> > >>>>>> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > >>>>>> possible to use fwupd in a Xen dom0, which is a serious problem for 
> > >>>>>> e.g.
> > >>>>>> Qubes OS.
> > >>>>>>
> > >>>>>> Before Xen 4.16, this was not fixable due to hypervisor limitations.
> > >>>>>> The UEFI specification requires the ESRT to be in EfiBootServicesData
> > >>>>>> memory, which Xen will use for whatever purposes it likes.  
> > >>>>>> Therefore,
> > >>>>>> Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > >>>>>>
> > >>>>>> Starting with Xen 4.17, Xen checks if the ESRT is in 
> > >>>>>> EfiBootServicesData
> > >>>>>> or EfiRuntimeServicesData memory.  If the ESRT is in 
> > >>>>>> EfiBootServicesData
> > >>>>>> memory, Xen allocates some memory of type EfiRuntimeServicesData, 
> > >>>>>> copies
> > >>>>>> the ESRT to it, and finally replaces the ESRT pointer with a pointer 
> > >>>>>> to
> > >>>>>> the copy.  Since Xen will not clobber EfiRuntimeServicesData memory,
> > >>>>>> this ensures that the ESRT can safely be accessed by the OS.  It is 
> > >>>>>> safe
> > >>>>>> to access the ESRT under Xen if, and only if, it is in memory of type
> > >>>>>> EfiRuntimeServicesData.
> > >>>>>>
> > >>>>>
> > >>>>> Thanks for the elaborate explanation. This is really helpful.
> > >>>>>
> > >>>>> So here, you are explaining that the only way for Xen to prevent
> > >>>>> itself from potentially clobbering the ESRT is by creating a
> > >>>>> completely new allocation?
> > >>>>
> > >>>> There are surely other ways, e.g. preserving BootServices* regions
> > >>>> alongside RuntimeServices* ones. But as the maintainer of the EFI
> > >>>> code in Xen I don't view this as a reasonable approach.
> > >>>
> > >>> Why not?
> > >>
> > >> Because it's against the intentions the EFI has (or at least had)
> > >> for this memory type. Much more than EfiAcpiReclaimMemory this
> > >> type is intended for use as ordinary RAM post-boot.
> > >
> > > What about giving that memory to dom0?  dom0’s balloon driver will give
> > > anything dom0 doesn’t wind up using back to Xen.
> >
> > While perhaps in principle possible, this would require special casing
> > in Xen. Except for the memory the initrd comes in, we don't directly
> > hand memory to Dom0. Instead everything goes through the page allocator
> > first. Plus if we really were convinced boot services memory needed
> > retaining, then it would also need retaining across kexec (and hence
> > shouldn't be left to Dom0 to decide what to do with it).
>
> So how should dom0 handle the various EFI tables other than the ESRT?
> Right now most uses of these tables in Linux are not guarded by any
> checks for efi_enabled(EFI_MEMMAP) or similar.  If some of them are in
> EfiBootServicesData memory, they might be corrupted before Linux gets
> them.

Yes, this is an annoying oversight of the EFI design: the config
tables are  tuples, and the size of the table is
specific to each table type. So without knowing the GUID, there is no
way you can reserve the right size.

Perhaps you could implement something like a hypercall in
efi_arch_mem_reserve(), which is called by the EFI code to reserve
regions that are in boot services memory but need to remain reserved?
This is used for all config tables that it knows or cares about.

Re: [PATCH v3] Support ESRT in Xen dom0

[Ard Biesheuvel]

On Tue, 20 Sept 2022 at 17:54, Jan Beulich  wrote:
>
> On 20.09.2022 17:36, Ard Biesheuvel wrote:
> > On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
> >  wrote:
> >>
> >> fwupd requires access to the EFI System Resource Table (ESRT) to
> >> discover which firmware can be updated by the OS.  Currently, Linux does
> >> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> >> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> >> Qubes OS.
> >>
> >> Before Xen 4.16, this was not fixable due to hypervisor limitations.
> >> The UEFI specification requires the ESRT to be in EfiBootServicesData
> >> memory, which Xen will use for whatever purposes it likes.  Therefore,
> >> Linux cannot safely access the ESRT, as Xen may have overwritten it.
> >>
> >> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> >> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> >> memory, Xen allocates some memory of type EfiRuntimeServicesData, copies
> >> the ESRT to it, and finally replaces the ESRT pointer with a pointer to
> >> the copy.  Since Xen will not clobber EfiRuntimeServicesData memory,
> >> this ensures that the ESRT can safely be accessed by the OS.  It is safe
> >> to access the ESRT under Xen if, and only if, it is in memory of type
> >> EfiRuntimeServicesData.
> >>
> >
> > Thanks for the elaborate explanation. This is really helpful.
> >
> > So here, you are explaining that the only way for Xen to prevent
> > itself from potentially clobbering the ESRT is by creating a
> > completely new allocation?
>
> There are surely other ways, e.g. preserving BootServices* regions
> alongside RuntimeServices* ones. But as the maintainer of the EFI
> code in Xen I don't view this as a reasonable approach.
>

Why not?

> > What about other assets that may be passed
> > via EFI boot services data regions?
>
> These would need handling similarly then.
>
> > So first of all, EfiRuntimeServicesData has a special purpose: it is
> > used to carry data that is part of the EFI runtime service
> > implementations themselves. Therefore, it has to be mapped into the
> > EFI page tables by the OS kernel, and carved out of the linear map (to
> > prevent inadvertent access with mismatched attributes). So unless you
> > are writing the code that backs GetVariable() or SetVariable(), there
> > are never good reasons to use EfiRuntimeServicesData.
>
> That's a rather strict interpretation of the spec. Even back when
> I started dealing with EFI, when it was still quite new, I know
> RuntimeServices* was already used for similar purposes.
>

I'm not saying it is never used inappropriately. But using a memory
type that gets mapped into the runtime services page tables every time
a runtime service call is made is pointless, fragments both the EFI
page tables as well as the kernel direct map for no reason.

> > If you want to use a memory type that is suitable for firmware tables
> > that are intended for consumption by the OS only (and not by the
> > runtime services themselves), you might consider EfiAcpiReclaimMemory.
>
> Personally I consider this type less appropriate than the one we
> currently use. It's intended to be used by ACPI, which doesn't
> come into play here.

In spite of the name, that is not really true. It is reclaimable
memory, which means that the OS can use the memory as conventional
memory after consuming its contents, or discarding them.

> It comes quite close to using e.g.
> EfiUnusableMemory here ...

No, that is something completely different. Using unusable memory for
anything would be silly.

> We might be able to (ab)use
> EfiLoaderData for this, but that would again require special
> casing (inside Xen) when deciding whether the memory can be used
> as general-purpose memory.
>

EFI loader data and EFI boot services data are the exact same thing
from this POV. They have no significance to the runtime firmware, and
can be used as ordinary available memory after ExitBootServices().

> > TBH I still don't think this is a scalable approach. There are other
> > configuration tables that may be passed in EFI boot services memory,
> > and MS especially were pushing back in the UEFI forum on adding table
> > types that were passed in anything other the EfiBootServicesData.
>
> Within Xen we might abstract the approach currently implemented in
> case more such pieces of data appear.
>
> While I can easily believe MS might be advocating for this model,
> I view it as problematic not only for Xen. How would you pass

Re: [PATCH v3] Support ESRT in Xen dom0

[Ard Biesheuvel]

Hello Demi,

On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
 wrote:
>
> fwupd requires access to the EFI System Resource Table (ESRT) to
> discover which firmware can be updated by the OS.  Currently, Linux does
> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> Qubes OS.
>
> Before Xen 4.16, this was not fixable due to hypervisor limitations.
> The UEFI specification requires the ESRT to be in EfiBootServicesData
> memory, which Xen will use for whatever purposes it likes.  Therefore,
> Linux cannot safely access the ESRT, as Xen may have overwritten it.
>
> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> memory, Xen allocates some memory of type EfiRuntimeServicesData, copies
> the ESRT to it, and finally replaces the ESRT pointer with a pointer to
> the copy.  Since Xen will not clobber EfiRuntimeServicesData memory,
> this ensures that the ESRT can safely be accessed by the OS.  It is safe
> to access the ESRT under Xen if, and only if, it is in memory of type
> EfiRuntimeServicesData.
>

Thanks for the elaborate explanation. This is really helpful.

So here, you are explaining that the only way for Xen to prevent
itself from potentially clobbering the ESRT is by creating a
completely new allocation? What about other assets that may be passed
via EFI boot services data regions?

So first of all, EfiRuntimeServicesData has a special purpose: it is
used to carry data that is part of the EFI runtime service
implementations themselves. Therefore, it has to be mapped into the
EFI page tables by the OS kernel, and carved out of the linear map (to
prevent inadvertent access with mismatched attributes). So unless you
are writing the code that backs GetVariable() or SetVariable(), there
are never good reasons to use EfiRuntimeServicesData.

If you want to use a memory type that is suitable for firmware tables
that are intended for consumption by the OS only (and not by the
runtime services themselves), you might consider EfiAcpiReclaimMemory.

TBH I still don't think this is a scalable approach. There are other
configuration tables that may be passed in EFI boot services memory,
and MS especially were pushing back in the UEFI forum on adding table
types that were passed in anything other the EfiBootServicesData.

> When running as a Xen dom0, check if the ESRT is in memory of type
> EfiRuntimeServicesData, and if it is, parse it as if not running under
> Xen.  This allows programs such as fwupd which require the ESRT to run
> under Xen, and so makes fwupd support in Qubes OS possible.
>
> Signed-off-by: Demi Marie Obenour 
> ---
> Changes since v2:
>
> - Massively updated commit message.
> - Fetch the ESRT inline in drivers/firmware/efi/esrt.c, instead of using
>   a single-use helper in drivers/xen/efi.c.
>
> Changes since v1:
>
> - Use a different type (struct xen_efi_mem_info) for memory information
>   provided by Xen, as Xen reports it in a different way than the
>   standard Linux functions do.
>
>  drivers/firmware/efi/esrt.c | 71 ++---
>  1 file changed, 58 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 
> 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..378bf2ea770ad3bd747345a89258216919eb21bb
>  100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -28,6 +28,11 @@
>  #include 
>  #include 
>
> +#ifdef CONFIG_XEN_EFI
> +#include 
> +#include 
> +#endif
> +
>  struct efi_system_resource_entry_v1 {
> efi_guid_t  fw_class;
> u32 fw_type;
> @@ -243,27 +248,67 @@ void __init efi_esrt_init(void)
> void *va;
> struct efi_system_resource_table tmpesrt;
> size_t size, max, entry_size, entries_size;
> -   efi_memory_desc_t md;
> -   int rc;
> phys_addr_t end;
> -
> -   if (!efi_enabled(EFI_MEMMAP))
> -   return;
> +   uint32_t type;
>
> pr_debug("esrt-init: loading.\n");
> if (!esrt_table_exists())
> return;
>
> -   rc = efi_mem_desc_lookup(efi.esrt, );
> -   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");
> +   if (efi_enabled(EFI_MEMMAP)) {
> +   efi_memory_desc_t md;
> +
> +   if (efi_mem_desc_lookup(efi.esrt, ) < 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;
> +   }
> +
> 

Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table

[Ard Biesheuvel]

On Tue, 6 Sept 2022 at 09:17, Leo Yan  wrote:
>
> Hi Marc,
>
> On Tue, Sep 06, 2022 at 07:27:17AM +0100, Marc Zyngier wrote:
> > On Tue, 06 Sep 2022 03:52:37 +0100,
> > Leo Yan  wrote:
> > >
> > > On Thu, Aug 25, 2022 at 10:40:41PM +0800, Leo Yan wrote:
> > >
> > > [...]
> > >
> > > > > > But here I still cannot create the concept that how GIC RD tables 
> > > > > > play
> > > > > > roles to support the para virtualization or passthrough mode.
> > > > >
> > > > > I am not sure what you are actually asking. The pending tables are 
> > > > > just
> > > > > memory you give to the GICv3 to record the state of the interrupts.
> > > >
> > > > For more specific, Xen has its own RD pending table, and we can use
> > > > this pending table to set state for SGI/PPI/LPI for a specific CPU
> > > > interface.  Xen works as hypervisor, it saves and restores the pending
> > > > table according to switched in VM context, right?
> > > >
> > > > On the other hand, what's the purpose for Linux kernel's GIC RD
> > > > pending table?  Is it only used for nested virtulisation?  I mean if
> > > > Linux kernel's GIC RD pending table is not used for the drivers in
> > > > Dom0 or DomU, then it's useless to pass it from the primary kernel to
> > > > secondary kernel; as result, we don't need to reserve the persistent
> > > > memory for the pending table in this case.
> > >
> > > I don't receive further confirmation from Marc, anyway, I tried to cook
> > > a kernel patch to mute the kernel oops [1].
> >
> > What sort of confirmation do you expect from me? None of what you
> > write above make much sense in the face of the architecture.
>
> Okay, I think have two questions for you:
>
> - The first question is if we really need to reserve persistent memory
>   for RD pending table and configuration table when Linux kernel runs
>   in Xen domain?
>
> - If the first question's answer is no, so it's not necessary to reserve
>   RD pending table and configuration table for Xen, then what's the good
>   way to dismiss the kernel oops?
>
> IIUC, you consider the general flow from architecture view, so you prefer
> to ask Xen to implement EFI stub to comply the general flow for EFI
> booting sequence, right?
>
> If the conclusion is to change Xen for support EFI stub, then this
> would be fine for me and I will hold on and leave Xen developers to work
> on it.
>

As I mentioned before, proper EFI boot support in Xen would be nice.
*However*, I don't think it makes sense to go through all the trouble
of implementing that just to shut up a warning that doesn't affect Xen
to begin with.


> > > [1] 
> > > https://lore.kernel.org/lkml/20220906024040.503764-1-leo@linaro.org/T/#u
> >
> > I'm totally baffled by the fact you're trying to add some extra hacks
> > to Linux just to paper over some of the Xen's own issues.
>
> I have a last question for why kernel reserves RD pending table and
> configuration table for kexec.  As we know, the primary kernel and
> the secondary kernel use separate memory regions,

This is only true for kdump, not for kexec in general.

> this means there have
> no race condition that secondary kernel modifies the tables whilist the
> GIC accesses the table if the secondary kernel allocates new pages for
> RD tables.  So only one potential issue I can image is the secondary
> kernel sets new RD pending table and configuration table, which might
> introduce inconsistent issue with rest RDs in the system.
>
> Could you confirm if my understanding is correct or not?
>
> Sorry for noise and many questions.  I understand this is a complex
> and difficult topic for me, and it's very likely that I am absent
> sufficient knowledge for this part, this is just what I want to
> learn from the discussion and from you :-)
>
> Thanks,
> Leo

Re: [PATCH v2] Add support for ESRT loading under Xen

[Ard Biesheuvel]

On Sun, 28 Aug 2022 at 04:52, Demi Marie Obenour
 wrote:
>
> This is needed for fwupd to work in Qubes OS.
>

Please elaborate on:
- the current situation
- why this is a problem
- why your approach is a reasonable solution.

> Signed-off-by: Demi Marie Obenour 
> ---
> Changes since v1:
>
> - Use a different type (struct xen_efi_mem_info) for memory information
>   provided by Xen, as Xen reports it in a different way than the
>   standard Linux functions do.
>
>  drivers/firmware/efi/esrt.c | 49 +++--
>  drivers/xen/efi.c   | 32 ++
>  include/linux/efi.h | 18 ++
>  3 files changed, 86 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 
> 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..c0fc149a838044cc16bb08a374a0c8ea6b7dcbff
>  100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -243,27 +243,50 @@ void __init efi_esrt_init(void)
> void *va;
> struct efi_system_resource_table tmpesrt;
> size_t size, max, entry_size, entries_size;
> -   efi_memory_desc_t md;
> -   int rc;
> phys_addr_t end;
> -
> -   if (!efi_enabled(EFI_MEMMAP))
> -   return;
> +   uint32_t type;
>
> pr_debug("esrt-init: loading.\n");
> if (!esrt_table_exists())
> return;
>
> -   rc = efi_mem_desc_lookup(efi.esrt, );
> -   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");
> +   if (efi_enabled(EFI_MEMMAP)) {
> +   efi_memory_desc_t md;
> +
> +   if (efi_mem_desc_lookup(efi.esrt, ) < 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;
> +   }
> +
> +   type = md.type;
> +   max = efi_mem_desc_end();
> +   } else if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) {
> +   struct xen_efi_mem_info info;
> +
> +   if (!xen_efi_mem_info_query(efi.esrt, )) {
> +   pr_warn("Failed to lookup ESRT header in Xen memory 
> map\n");
> +   return;
> +   }
> +
> +   type = info.type;
> +   max = info.addr + info.size;
> +
> +   /* Recent Xen versions relocate the ESRT to memory of type
> +* EfiRuntimeServicesData, which Xen will not reuse.  If the 
> ESRT

This violates the EFI spec, which spells out very clearly that the
ESRT must be in EfiBootServicesData memory. Why are you deviating from
this?

> +* is not in EfiRuntimeServicesData memory, it has not been 
> reserved
> +* by Xen and might be allocated to other guests, so it cannot
> +* safely be used. */
> +   if (type != EFI_RUNTIME_SERVICES_DATA) {
> +   pr_warn("Xen did not reserve ESRT, ignoring it\n");
> +   return;
> +   }
> +   } else {
> return;
> }
>
> -   max = efi_mem_desc_end();
> if (max < efi.esrt) {
> pr_err("EFI memory descriptor is invalid. (esrt: %p max: 
> %p)\n",
>(void *)efi.esrt, (void *)max);
> @@ -333,7 +356,7 @@ void __init efi_esrt_init(void)
>
> end = esrt_data + size;
> pr_info("Reserving ESRT space from %pa to %pa.\n", _data, );
> -   if (md.type == EFI_BOOT_SERVICES_DATA)
> +   if (type == EFI_BOOT_SERVICES_DATA)
> efi_mem_reserve(esrt_data, esrt_data_size);
>
> pr_debug("esrt-init: loaded.\n");
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index 
> d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..b313f213822f0fd5ba6448f6f6f453cfda4c7e23
>  100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -26,6 +26,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -40,6 +41,37 @@
>
>  #define efi_data(op)   (op.u.efi_runtime_call)
>
> +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> +  "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> +
> +bool xen_efi_mem_info_query(u64 phys_addr, struct xen_efi_mem_info *md)
> +{
> +   struct xen_platform_op op = {
> +   .cmd = XENPF_firmware_info,
> +   .u.firmware_info = {
> +   .type = XEN_FW_EFI_INFO,
> +   .index = XEN_FW_EFI_MEM_INFO,
> +   .u.efi_info.mem.addr = phys_addr,
> +   .u.efi_info.mem.size = ((u64)-1ULL) - phys_addr,

Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table

[Ard Biesheuvel]

On Thu, 18 Aug 2022 at 17:49, Leo Yan  wrote:
>
> On Thu, Aug 18, 2022 at 11:04:48AM +0100, Julien Grall wrote:
>
> [...]
>
> > > > Seems it's broken for kdump/kexec if kernel boots with using DT?
> > > >
> > >
> > > EFI supports both DT and ACPI boot, but only ACPI *requires* EFI.
> > >
> > > So DT boot on hardware with affected GICv3 implementations works fine
> > > with kdump/kexec as long as EFI is being used. Using non-EFI boot and
> > > kexec on such systems will likely result in a splat on the second
> > > kernel, unless there is another mechanism being used to reserve the
> > > memory in DT as well.
> > >
> > > Maybe we should wire up the EFI_PARAVIRT flag for Xen dom0 on arm64,
> > > so that we can filter out the call above. That would mean that
> > > Xen/arm64/dom0/kexec on affected hardware would result in a splat in
> > > the second kernel as well, but whether that matters depends on your
> > > support scope.
> >
> > In the context of Xen, dom0 doesn't have direct access to the host ITS
> > because we are emulating it. So I think it doesn't matter for us because we
> > can fix our implementation if it is affected.
> >
> > That said, kexec-ing dom0 (or any other domain) on Xen on Arm would require
> > some work to be supported. OOI, @leo is it something you are investigating?
>
>
> Now I am working on automative reference platform; the first thing for
> me is to resolve the kernel oops.
>
> For long term, I think the kexec/kdump would be important to be
> supported, to be clear, so far supporting kexec/kdump for Xen/Linux is
> not priority for my work.
>
> Also thanks a lot for Ard and Mark's replying. To be honest, I missed
> many prerequisites (e.g. redistributor configurations for GIC in
> hypervisor) and seems Xen uses a different way by emulating GICv3
> controller for guest OS.  So now I am bit puzzle what's for next step
> or just keep as it is?
>

If i understand Julien's remark correctly, the dom0 GICv3 is emulated,
and so it should not suffer from the issue that we are working around
here.

Given that this workaround is still the sole user of the MEMRESERVE
API, we would like to remain free to rip it out and replace it
completely if we need to, and so implementing it in Xen is a bad idea,
especially if the issue in question does not even exist in your case.
This means that even if you do decide to support kexec, things should
work fine in spite of this warning regarding the failure to perform
the memory reservation, as the GIC can simply be reconfigured.

So perhaps we should just [conditionally] tone down the warning?

Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table

[Ard Biesheuvel]

On Thu, 18 Aug 2022 at 11:15, Leo Yan  wrote:
>
> Hi Ard,
>
> On Wed, Aug 17, 2022 at 03:49:32PM +0200, Ard Biesheuvel wrote:
>
> [...]
>
> > > This header holds ACPI spec defined data structures. This one looks
> > > to be a Linux one, and hence shouldn't be defined here. You use it
> > > in a single CU only, so I see no reason to define it there.
> > >
> > > Furthermore - what if Linux decided to change their structure? Or
> > > is there a guarantee that they won't?
> >
> > No, there is not. The memreserve table is an internal interface
> > between the EFI stub loader and the Linux kernel proper.
> >
> > As I have argued many times before, booting the arm64 kernel in
> > EFI/ACPI mode without going through the EFI stub violates the ACPI
> > spec, and relies on interfaces that were not intended for public
> > consumption.
>
> Let me ask a question but sorry it might be off topic.
>
> In the Linux kernel function:
>
>   static int gic_reserve_range(phys_addr_t addr, unsigned long size)
>   {
>   if (efi_enabled(EFI_CONFIG_TABLES))
>   return efi_mem_reserve_persistent(addr, size);
>
>   return 0;
>   }
>
> We can see it will reserve persistent memory region for GIC pending
> pages, so my question is if a platform is booting with DT binding
> rather than ACPI table, how the primary kernel can reserve the pages
> and pass the info to the secondary kernel?
>
> Seems it's broken for kdump/kexec if kernel boots with using DT?
>

EFI supports both DT and ACPI boot, but only ACPI *requires* EFI.

So DT boot on hardware with affected GICv3 implementations works fine
with kdump/kexec as long as EFI is being used. Using non-EFI boot and
kexec on such systems will likely result in a splat on the second
kernel, unless there is another mechanism being used to reserve the
memory in DT as well.

Maybe we should wire up the EFI_PARAVIRT flag for Xen dom0 on arm64,
so that we can filter out the call above. That would mean that
Xen/arm64/dom0/kexec on affected hardware would result in a splat in
the second kernel as well, but whether that matters depends on your
support scope.

Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table

[Ard Biesheuvel]

On Wed, 17 Aug 2022 at 15:18, Jan Beulich  wrote:
>
> On 17.08.2022 12:57, Leo Yan wrote:
> > On Arm64, when boot Dom0 with the Linux kernel, it reports the warning:
> >
> > [0.403737] [ cut here ]
> > [0.403738] WARNING: CPU: 30 PID: 0 at 
> > drivers/irqchip/irq-gic-v3-its.c:3074 its_cpu_init+0x814/0xae0
> > [0.403745] Modules linked in:
> > [0.403748] CPU: 30 PID: 0 Comm: swapper/30 Tainted: GW 
> > 5.15.23-ampere-lts-standard #1
> > [0.403752] pstate: 61c5 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS 
> > BTYPE=--)
> > [0.403755] pc : its_cpu_init+0x814/0xae0
> > [0.403758] lr : its_cpu_init+0x810/0xae0
> > [0.403761] sp : 89c03ce0
> > [0.403762] x29: 89c03ce0 x28: 001e x27: 
> > 880711f43000
> > [0.403767] x26: 8a3c0070 x25: fc1ffe0a4400 x24: 
> > 8a3c
> > [0.403770] x23: 895bc998 x22: 890a6000 x21: 
> > 89850cb0
> > [0.403774] x20: 89701a10 x19: 89701000 x18: 
> > 
> > [0.403777] x17: 3030303035303031 x16: 3030313030303078 x15: 
> > 303a30206e6f6967
> > [0.403780] x14: 6572206530312072 x13: 3030303030353030 x12: 
> > 3130303130303030
> > [0.403784] x11: 78303a30206e6f69 x10: 6765722065303120 x9 : 
> > 8870e710
> > [0.403788] x8 : 6964657220646e75 x7 : 0003 x6 : 
> > 
> > [0.403791] x5 :  x4 : fc00 x3 : 
> > 0010
> > [0.403794] x2 :  x1 : 0001 x0 : 
> > ffed
> > [0.403798] Call trace:
> > [0.403799]  its_cpu_init+0x814/0xae0
> > [0.403802]  gic_starting_cpu+0x48/0x90
> > [0.403805]  cpuhp_invoke_callback+0x16c/0x5b0
> > [0.403808]  cpuhp_invoke_callback_range+0x78/0xf0
> > [0.403811]  notify_cpu_starting+0xbc/0xdc
> > [0.403814]  secondary_start_kernel+0xe0/0x170
> > [0.403817]  __secondary_switched+0x94/0x98
> > [0.403821] ---[ end trace f68728a0d3053b70 ]---
> >
> > In the Linux kernel, the GIC driver tries to reserve ITS interrupt
> > table, and the reserved pages can survive for kexec/kdump and will be
> > used for secondary kernel.  Linux kernel relies on MEMRESERVE EFI
> > configuration table for memory page , but this table is not supported
> > by Xen.
> >
> > This patch adds a MEMRESERVE configuration table into the system table,
> > it points to a dummy data structure acpi_table_memreserve, this
> > structure has the consistent definition with the Linux kernel.
> > Following the method in Xen, it creates a table entry for the structure
> > acpi_table_memreserve.
> >
> > Cc: Ard Biesheuvel 
> > Cc: Marc Zyngier 
> > Cc: Bertrand Marquis 
> > Cc: Rahul Singh 
> > Cc: Peter Griffin 
> > Signed-off-by: Leo Yan 
> > ---
> >  xen/arch/arm/acpi/domain_build.c | 24 
> >  xen/arch/arm/efi/efi-dom0.c  | 19 ---
> >  xen/arch/arm/include/asm/acpi.h  |  1 +
> >  xen/include/acpi/actbl.h | 17 +
> >  xen/include/efi/efiapi.h |  2 ++
> >  5 files changed, 60 insertions(+), 3 deletions(-)
>
...
> > --- a/xen/include/acpi/actbl.h
> > +++ b/xen/include/acpi/actbl.h
> > @@ -302,6 +302,23 @@ struct acpi_table_fadt {
> >  #define ACPI_FADT_HW_REDUCED(1<<20)  /* 20: [V5] ACPI hardware is 
> > not implemented (ACPI 5.0) */
> >  #define ACPI_FADT_LOW_POWER_S0  (1<<21)  /* 21: [V5] S0 power savings 
> > are equal or better than S3 (ACPI 5.0) */
> >
> > +/***
> > + *
> > + * MEMRESERVE - Dummy entry for memory reserve configuration table
> > + *
> > + 
> > **/
> > +
> > +struct acpi_table_memreserve {
> > + int size;   /* allocated size of the array */
> > + int count;  /* number of entries used */
> > + u64 next;   /* pa of next struct instance */
> > + struct {
> > + u64 base;
> > + u64 size;
> > + } entry[];
> > +};
>
> This header holds ACPI spec defined data structures. This one looks
> to be a Linux one, and hence shouldn't be defined here. You use it
> in a single CU only, so I see no reason to define it there.
>
> Furthermore - what if Linux

Re: Question: Enable LINUX_EFI_MEMRESERVE_TABLE_GUID in EFI

[Ard Biesheuvel]

On Thu, 11 Aug 2022 at 16:59, Bertrand Marquis  wrote:
>
> Hi Leon,
>
> > On 11 Aug 2022, at 15:17, Leo Yan  wrote:
> >
> > Hi Bertrand, Rahul,
> >
> > On Fri, Aug 05, 2022 at 12:05:23PM +, Bertrand Marquis wrote:
> >>> On 5 Aug 2022, at 12:44, Rahul Singh  wrote:
> >
> > [...]
> >
>  Looked into the code, the GICv3 driver tries to create persistent
>  reservations for pending pages, and the persistent reservation table
>  can be used by kexec/kdump.  For the persistent reservations, it
>  relies on MEMRESERVE EFI configuration table, but this table is not
>  supported by xen.efi, I think this is the reason for the above oops.
> >>>
> >>> Yes, you are right above warning is observed because Xen does not support
> >>> memreserve efi table. I also observed a similar warning on the N1SDP 
> >>> board.
> 
>  I checked that if I boot a host Linux (without Xen), then the EFI has
>  provided MEMRESERVE configuration table, I can get below log:
> 
>  #  dmesg | grep MEMRESERVE
>  [0.00] efi: TPMFinalLog=0x807f9ef ACPI 2.0=0x807fa0d0018 ... 
>  MEMRESERVE=0x807f8141e98
> 
>  Just want to confirm, is anyone working on enabling MEMRESERVE EFI
>  configuration table for Xen?  And welcome any comments and
>  suggestions!
> 
> >>
> >> No I do not think anybody is working on this at the moment.
> >> If you want to work on adding support for this in Xen, we can provide 
> >> support
> >> and help on reviewing and testing as we have several targets on which we
> >> observe this (N1SDP and Ava).
> >
> > Thanks for your quick response.
> >
> > I took time to look into the code, below are my conclusions.
> >
> > For a normal UEFI boot flow, UEFI will invoke Linux kernel's EFI stub,
> > and the EFI stub will install MEMRESERVE EFI configuration table.
> > This is accomplished in the Linux function install_memreserve_table().
> >
> > Secondly, Xen passes DT to kernel, it synthesizes ACPI compatible
> > nodes in the device tree and finally kernel parses DT and create the
> > ACPI table.  In this case, Xen doesn't invoke Linux EFI stub.
> >
> > To be honest, I have very less knowledge for Xen and APCI; just based on
> > reading code, I think it's hard for Xen to invoke Linux kernel's EFI
> > stub, this is because Xen needs to provide the EFI runtime services, and
> > I don't think it's feasible for Xen to pass through UEFI's runtime
> > service to Linux kernel.  If we implement the EFI runtime services for
> > Xen, then this would introduce a big implementation.
> >
> > So another option is we simply add MEMRESERVE EFI configuration table
> > into device tree, just like Xen does for other ACPI tables (e.g.
> > RSDP?).  And then in Linux kernel, we need to parse the DT binding and
> > initialize the corresponding variables in kernel, so we need to add
> > support in the Linux source drivers/firmware/efi/fdtparams.c.
> >
> > Before I proceed, just want to check which option would be the right
> > way to move forward?  And I am open for any other solution and welcome
> > suggestions.
>
> When Xen is started using EFI, Linux is then started purely using device tree
> there is a standard way to define reserved memory to linux using the device
> tree and Xen should decode the Memreserve entry from EFI and add the
> corresponding entry in the device tree that we give to linux.
>

This is not what MEMRESERVE is used for. Specifying reservations for
the current boot is straight-forward. What MEMRESERVE does is specify
a reservation that survives kexec and is passed on to the next
kernel(s), as the table is anchored in a structure that is created by
the EFI stub on the first boot. This is needed for the GICv3 on some
platforms, as memory that Linux reserves for its interrupt tables can
never be released again, even across kexec, which means that the GICv3
will be DMA'ing into that memory if the kexec kernel wants it or not)

I'd strongly recommend against doing any of the things Xen does for
ACPI boot today: both the ACPI spec and the kernel documentation about
ACPI support in the arm64 port is 100% clear that EFI boot is the only
supported boot method. Issues like this one would have never popped up
if those rules were adhered to. (/pedantic mode off)

In your case, this is a matter of allocating a structure of the right
type and size, and making it available via the configuration table
array in the EFI system table that the dom0 kernel receives from Xen
at boot.

Please don't add DT entries for this - we should be able to cover this
using the existing pseudo-EFI boot flow that Xen uses today.

-- 
Ard.

Re: [PATCH] efi: x86/xen: switch to efi_get_secureboot_mode helper

[Ard Biesheuvel]

On Thu, 5 Nov 2020 at 00:53,  wrote:
>
>
> On 11/4/20 5:14 PM, Ard Biesheuvel wrote:
> > Now that we have a static inline helper to discover the platform's secure
> > boot mode that can be shared between the EFI stub and the kernel proper,
> > switch to it, and drop some comments about keeping them in sync manually.
> >
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  arch/x86/xen/efi.c| 37 +---
> >  drivers/firmware/efi/libstub/secureboot.c |  3 --
> >  2 files changed, 9 insertions(+), 31 deletions(-)
> >
> > diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
> > index 205a9bc981b0..a27444acaf1e 100644
> > --- a/arch/x86/xen/efi.c
> > +++ b/arch/x86/xen/efi.c
> > @@ -93,37 +93,22 @@ static efi_system_table_t __init *xen_efi_probe(void)
> >
> >  /*
> >   * Determine whether we're in secure boot mode.
> > - *
> > - * Please keep the logic in sync with
> > - * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
> >   */
> >  static enum efi_secureboot_mode xen_efi_get_secureboot(void)
> >  {
> > - static efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
> >   static efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> > + enum efi_secureboot_mode mode;
> >   efi_status_t status;
> > - u8 moksbstate, secboot, setupmode;
> > + u8 moksbstate;
> >   unsigned long size;
> >
> > - size = sizeof(secboot);
> > - status = efi.get_variable(L"SecureBoot", _variable_guid,
> > -   NULL, , );
> > -
> > - if (status == EFI_NOT_FOUND)
> > - return efi_secureboot_mode_disabled;
> > -
> > - if (status != EFI_SUCCESS)
> > - goto out_efi_err;
> > -
> > - size = sizeof(setupmode);
> > - status = efi.get_variable(L"SetupMode", _variable_guid,
> > -   NULL, , );
> > -
> > - if (status != EFI_SUCCESS)
> > - goto out_efi_err;
> > -
> > - if (secboot == 0 || setupmode == 1)
> > - return efi_secureboot_mode_disabled;
> > + mode = efi_get_secureboot_mode(efi.get_variable);
>
>
> Which tree is this patch against? I don't see a definition of 
> efi_get_secureboot_mode().
>
>
> > + if (mode == efi_secureboot_mode_unknown) {
> > + efi_err("Could not determine UEFI Secure Boot status.\n");
>
>
> We need to include drivers/firmware/efi/libstub/efistub.h for that. Which I 
> am not sure is meant to be included anywhere outside of libstub.
>

Ah yes, my mistake - that should be pr_err, not efi_err.

[PATCH] efi: x86/xen: switch to efi_get_secureboot_mode helper

[Ard Biesheuvel]

Now that we have a static inline helper to discover the platform's secure
boot mode that can be shared between the EFI stub and the kernel proper,
switch to it, and drop some comments about keeping them in sync manually.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/xen/efi.c| 37 +---
 drivers/firmware/efi/libstub/secureboot.c |  3 --
 2 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 205a9bc981b0..a27444acaf1e 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -93,37 +93,22 @@ static efi_system_table_t __init *xen_efi_probe(void)
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
  */
 static enum efi_secureboot_mode xen_efi_get_secureboot(void)
 {
-   static efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
static efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
+   enum efi_secureboot_mode mode;
efi_status_t status;
-   u8 moksbstate, secboot, setupmode;
+   u8 moksbstate;
unsigned long size;
 
-   size = sizeof(secboot);
-   status = efi.get_variable(L"SecureBoot", _variable_guid,
- NULL, , );
-
-   if (status == EFI_NOT_FOUND)
-   return efi_secureboot_mode_disabled;
-
-   if (status != EFI_SUCCESS)
-   goto out_efi_err;
-
-   size = sizeof(setupmode);
-   status = efi.get_variable(L"SetupMode", _variable_guid,
- NULL, , );
-
-   if (status != EFI_SUCCESS)
-   goto out_efi_err;
-
-   if (secboot == 0 || setupmode == 1)
-   return efi_secureboot_mode_disabled;
+   mode = efi_get_secureboot_mode(efi.get_variable);
+   if (mode == efi_secureboot_mode_unknown) {
+   efi_err("Could not determine UEFI Secure Boot status.\n");
+   return efi_secureboot_mode_unknown;
+   }
+   if (mode != efi_secureboot_mode_enabled)
+   return mode;
 
/* See if a user has put the shim into insecure mode. */
size = sizeof(moksbstate);
@@ -140,10 +125,6 @@ static enum efi_secureboot_mode 
xen_efi_get_secureboot(void)
  secure_boot_enabled:
pr_info("UEFI Secure Boot is enabled.\n");
return efi_secureboot_mode_enabled;
-
- out_efi_err:
-   pr_err("Could not determine UEFI Secure Boot status.\n");
-   return efi_secureboot_mode_unknown;
 }
 
 void __init xen_efi_init(struct boot_params *boot_params)
diff --git a/drivers/firmware/efi/libstub/secureboot.c 
b/drivers/firmware/efi/libstub/secureboot.c
index af18d86c1604..8a18930f3eb6 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -24,9 +24,6 @@ static efi_status_t get_var(efi_char16_t *name, efi_guid_t 
*vendor, u32 *attr,
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * arch/x86/xen/efi.c:xen_efi_get_secureboot().
  */
 enum efi_secureboot_mode efi_get_secureboot(void)
 {
-- 
2.17.1

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

[Ard Biesheuvel]

On Thu, 20 Aug 2020 at 11:30, Roger Pau Monné  wrote:
>
> On Wed, Aug 19, 2020 at 01:33:39PM +0200, Norbert Kaminski wrote:
> >
> > On 19.08.2020 10:19, Roger Pau Monné wrote:
> > > On Tue, Aug 18, 2020 at 08:40:18PM +0200, Marek Marczykowski-Górecki 
> > > wrote:
> > > > On Tue, Aug 18, 2020 at 07:21:14PM +0200, Roger Pau Monné wrote:
> > > > > > Let me draw the picture from the beginning.
> > > > > Thanks, greatly appreciated.
> > > > >
> > > > > > EFI memory map contains various memory regions. Some of them are 
> > > > > > marked
> > > > > > as not needed after ExitBootServices() call (done in Xen before
> > > > > > launching dom0). This includes EFI_BOOT_SERVICES_DATA and
> > > > > > EFI_BOOT_SERVICES_CODE.
> > > > > >
> > > > > > EFI SystemTable contains pointers to various ConfigurationTables -
> > > > > > physical addresses (at least in this case). Xen does interpret some 
> > > > > > of
> > > > > > them, but not ESRT. Xen pass the whole (address of) SystemTable to 
> > > > > > Linux
> > > > > > dom0 (at least in PV case). Xen doesn't do anything about tables it
> > > > > > doesn't understand.
> > > > > >
> > > > > > Now, the code in Linux takes the (ESRT) table address early and 
> > > > > > checks
> > > > > > the memory map for it. We have 3 cases:
> > > > > >   - it points at area marked as neither EFI_*_SERVICES_DATA, nor 
> > > > > > with
> > > > > > EFI_MEMORY_RUNTIME attribute -> Linux refuse to use it
> > > > > >   - it points to EFI_RUNTIME_SERVICES_DATA or with 
> > > > > > EFI_MEMORY_RUNTIME
> > > > > > attribute - Linux uses the table; memory map already says the 
> > > > > > area
> > > > > > belongs to EFI and the OS should not use it for something else
> > > > > >   - it points to EFI_BOOT_SERVICES_DATA - Linux mark the area as 
> > > > > > reserved
> > > > > > to not release it after calling ExitBootServices()
> > > > > >
> > > > > > The problematic is the third case - at the time when Linux dom0 is 
> > > > > > run,
> > > > > > ExitBootServices() was already called and EFI_BOOT_SERVICES_* 
> > > > > > memory was
> > > > > > already released. It could be already used for something else (for
> > > > > > example Xen could overwrite it while loading dom0).
> > > > > >
> > > > > > Note the problematic case should be the most common - UEFI 
> > > > > > specification
> > > > > > says "The ESRT shall be stored in memory of type 
> > > > > > EfiBootServicesData"
> > > > > > (chapter 22.3 of UEFI Spec v2.6).
> > > > > >
> > > > > > For this reason, to use ESRT in dom0, Xen should do something about 
> > > > > > it
> > > > > > before ExitBootServices() call. While analyzing all the EFI tables 
> > > > > > is
> > > > > > probably not a viable option, it can do some simple action:
> > > > > >   - retains all the EFI_BOOT_SERVICES_* areas - there is already 
> > > > > > code
> > > > > > for that, controlled with /mapbs boot switch (to xen.efi, would 
> > > > > > need
> > > > > > another option for multiboot2+efi)
> > > > > >   - have a list of tables to retain - since Xen already do analyze 
> > > > > > some
> > > > > > of the ConfigurationTables, it can also have a list of those to
> > > > > > preserve even if they live in EFI_BOOT_SERVICES_DATA. In this 
> > > > > > case,
> > > > > > while Xen doesn't need to parse the whole table, it need to 
> > > > > > parse it's
> > > > > > header to get the table size - to reserve that memory and not 
> > > > > > reuse
> > > > > > it after ExitBootServices().
> > > > > Xen seems to already contain skeleton
> > > > > XEN_EFI_query_capsule_capabilities and XEN_EFI_update_capsule
> > > > > hypercalls which is what should be used in order to perform the
> > > > > updates?
> > > > I think those covers only runtime service calls similarly named. But you
> > > > need also ESRT table to collect info about devices that you can even
> > > > attempt to update.
> > > Right, the ESRT must be available so that dom0 can discover the
> > > resources.
> > >
> > > > TBH, I'm not sure if those runtime services are really needed. I think
> > > > Norbert succeeded UEFI update from within Xen PV dom0 with just access
> > > > to the ESRT table, but without those services.
> > > >
> > Marek is right here. I was able to successfully update and downgrade
> > UFEI when the ESRT table was provided to the Xen PV dom0. I didn't
> > need any extra services to make the UEFI capsule update work.
>
> OK, I think that's using the method described in 8.5.5 of delivery of
> Capsules via file on Mass Storage, which doesn't use the
> UpdateCapsule() runtime API?
>

No, it doesn't even do that. It uses its own .efi binary to invoke
UpdateCapsule() after a reboot, by setting up the BootNext variable to
override the boot target for the next boot only.

> Using such method doesn't require QueryCapsuleCapabilities(), as
> that's used to know whether a certain capsule can be updated via
> UpdateCapsule().
>

That is a bit of a downside here. But the reason 

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

[Ard Biesheuvel]

Hi Marek,

On Sun, 16 Aug 2020 at 02:20, Marek Marczykowski-Górecki
 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 
> ---
>  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, );
> -   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, );
> +   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();
> -   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();
> +   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", _data, );
> -   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
>

Re: [PATCH v2] efi: avoid error message when booting under Xen

[Ard Biesheuvel]

On Fri, 10 Jul 2020 at 17:24, Juergen Gross  wrote:
>
> efifb_probe() will issue an error message in case the kernel is booted
> as Xen dom0 from UEFI as EFI_MEMMAP won't be set in this case. Avoid
> that message by calling efi_mem_desc_lookup() only if EFI_MEMMAP is set.
>
> Fixes: 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes when 
> mapping the FB")
> Signed-off-by: Juergen Gross 

Acked-by: Ard Biesheuvel 

> ---
>  drivers/video/fbdev/efifb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 65491ae74808..e57c00824965 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -453,7 +453,7 @@ static int efifb_probe(struct platform_device *dev)
> info->apertures->ranges[0].base = efifb_fix.smem_start;
> info->apertures->ranges[0].size = size_remap;
>
> -   if (efi_enabled(EFI_BOOT) &&
> +   if (efi_enabled(EFI_MEMMAP) &&
> !efi_mem_desc_lookup(efifb_fix.smem_start, )) {
> if ((efifb_fix.smem_start + efifb_fix.smem_len) >
> (md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT))) {
> --
> 2.26.2
>

Re: [PATCH] efi: avoid error message when booting under Xen

[Ard Biesheuvel]

On Fri, 10 Jul 2020 at 16:38, Jürgen Groß  wrote:
>
> On 10.07.20 15:27, Ard Biesheuvel wrote:
> > On Fri, 10 Jul 2020 at 13:17, Bartlomiej Zolnierkiewicz
> >  wrote:
> >>
> >>
> >> [ added EFI Maintainer & ML to Cc: ]
> >>
> >> Hi,
> >>
> >> On 7/9/20 11:17 AM, Jürgen Groß wrote:
> >>> On 28.06.20 10:50, Jürgen Groß wrote:
> >>>> Ping?
> >>>>
> >>>> On 10.06.20 16:10, Juergen Gross wrote:
> >>>>> efifb_probe() will issue an error message in case the kernel is booted
> >>>>> as Xen dom0 from UEFI as EFI_MEMMAP won't be set in this case. Avoid
> >>>>> that message by calling efi_mem_desc_lookup() only if EFI_PARAVIRT
> >>>>> isn't set.
> >>>>>
> >
> > Why not test for EFI_MEMMAP instead of EFI_BOOT?
>
> Honestly I'm not sure EFI_BOOT is always set in that case. If you tell
> me it is fine to just replace the test to check for EFI_MEMMAP I'm fine
> to modify my patch.
>


Yes please

Re: [PATCH] efi: avoid error message when booting under Xen

[Ard Biesheuvel]

On Fri, 10 Jul 2020 at 13:17, Bartlomiej Zolnierkiewicz
 wrote:
>
>
> [ added EFI Maintainer & ML to Cc: ]
>
> Hi,
>
> On 7/9/20 11:17 AM, Jürgen Groß wrote:
> > On 28.06.20 10:50, Jürgen Groß wrote:
> >> Ping?
> >>
> >> On 10.06.20 16:10, Juergen Gross wrote:
> >>> efifb_probe() will issue an error message in case the kernel is booted
> >>> as Xen dom0 from UEFI as EFI_MEMMAP won't be set in this case. Avoid
> >>> that message by calling efi_mem_desc_lookup() only if EFI_PARAVIRT
> >>> isn't set.
> >>>

Why not test for EFI_MEMMAP instead of EFI_BOOT?


> >>> Fixes: 38ac0287b7f4 ("fbdev/efifb: Honour UEFI memory map attributes when 
> >>> mapping the FB")
> >>> Signed-off-by: Juergen Gross 
> >>> ---
> >>>   drivers/video/fbdev/efifb.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> >>> index 65491ae74808..f5eccd1373e9 100644
> >>> --- a/drivers/video/fbdev/efifb.c
> >>> +++ b/drivers/video/fbdev/efifb.c
> >>> @@ -453,7 +453,7 @@ static int efifb_probe(struct platform_device *dev)
> >>>   info->apertures->ranges[0].base = efifb_fix.smem_start;
> >>>   info->apertures->ranges[0].size = size_remap;
> >>> -if (efi_enabled(EFI_BOOT) &&
> >>> +if (efi_enabled(EFI_BOOT) && !efi_enabled(EFI_PARAVIRT) &&
> >>>   !efi_mem_desc_lookup(efifb_fix.smem_start, )) {
> >>>   if ((efifb_fix.smem_start + efifb_fix.smem_len) >
> >>>   (md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT))) {
> >>>
> >>
> >
> > In case I see no reaction from the maintainer for another week I'll take
> > this patch through the Xen tree.
>
> From fbdev POV this change looks fine to me and I'm OK with merging it
> through Xen or EFI tree:
>
> Acked-by: Bartlomiej Zolnierkiewicz 
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R Institute Poland
> Samsung Electronics

Re: [RFC] UEFI Secure Boot on Xen Hosts

[Ard Biesheuvel]

On Tue, 5 May 2020 at 23:58, Bobby Eshleman  wrote:
>
> On Thu, Apr 30, 2020 at 01:41:12PM +0200, Ard Biesheuvel wrote:
> > On Thu, 30 Apr 2020 at 13:15, Daniel Kiper  wrote:
> > > Anyway, the advantage of this new protocol is that it uses UEFI API to
> > > load and execute PE kernel and its modules. This means that it will be
> > > architecture independent. However, IIRC, if we want to add new modules
> > > types to the Xen then we have to teach all bootloaders in the wild about
> > > new GUIDs. Ard, am I correct?
> > >
> >
> > Well, if you are passing multiple files that are not interchangeable,
> > each bootloader will need to do something, right? It could be another
> > GUID, or we could extend the initrd media device path to take
> > discriminators.
> >
> > So today, we have
> >
> > VendorMedia(5568e427-68fc-4f3d-ac74-ca555231cc68)
> >
> > which identifies /the/ initrd on Linux. As long as this keeps working
> > as intended, I have no objections extending this to
> >
> > VendorMedia(5568e427-68fc-4f3d-ac74-ca555231cc68)/File(...)
> >
> > etc, if we can agree that omitting the File() part means the unnamed
> > initrd, and that L"initrd" is reserved as a file name. That way, you
> > can choose any abstract file name you want, but please note that the
> > loader still needs to provide some kind of mapping of how these
> > abstract file names relate to actual files selected by the user.
>
> This seems reasonable to me and I can't think of any good reason right
> now, if we go down this path, to not just extend the initrd media device
> path (as opposed to creating one that is Xen-specific).  It definitely
> beats having a GUID per boot module since the number of modules changes
> per Xen deployment, so there would need to be some range of GUIDs
> representing modules specifically for Xen, and some rules as to how they
> are mapped to real files.
>
> It does seem simpler to ask the loader for "dom0's kernel" or "dom1's
> initrd" and to have the loader use these references to grab real files.
>

Yes. And note that using a single GUID + File component is easier on
the implementation too: the protocol implementation has to be
registered only once, and the single callback that exists will be
invoked with different values for 'FilePath', corresponding with
different values for File(). For example, in [0], this maps to the
check at line 120, where we currently only consider the
'end-of-device-path' terminator type to be valid, but this could
easily be extended to parse the contents of a subsequent file node and
resolve it to grab some actual contents.

[0] 
https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0


> > One thing to keep in mind, though, is that this protocol goes away
> > after ExitBootServices().
> >
>
> Roger that.
>
>
> Thanks,
>
> -Bobby

Re: [RFC] UEFI Secure Boot on Xen Hosts

[Ard Biesheuvel]

On Thu, 30 Apr 2020 at 13:15, Daniel Kiper  wrote:
>
> Adding Ard...
>
> On Thu, Apr 30, 2020 at 09:01:45AM +0200, Jan Beulich wrote:
> > On 30.04.2020 00:51, Bobby Eshleman wrote:
> > > Hey all,
> > >
> > > We're looking to develop UEFI Secure Boot support for grub-loaded Xen and
> > > ultimately for XCP-ng (I'm on the XCP-ng team at Vates).
> > >
> > > In addition to carrying the chain-of-trust through the entire boot 
> > > sequence
> > > into dom0, we would also like to build something akin to Linux's Lockdown 
> > > for
> > > dom0 and its privileged interfaces.
> > >
> > > It appears that there are various options and we'd like to discuss them 
> > > with
> > > the community.
> > >
> > > # Option #1: PE/COFF and Shim
> > >
> > > Shim installs a verification protocol available to subsequent programs 
> > > via EFI
> > > boot services.  The protocol is called SHIM_LOCK and it is currently 
> > > supported
> > > by xen.efi.
> > >
> > > Shim requires the payload under verification to be a PE/COFF executable.  
> > > In
> > > order to support both shim and maintain the multiboot2 protocol, Daniel 
> > > Kiper's
> > > patchset [1]  (among other things) incorporates the PE/COFF header
> > > into xen.gz and adds dom0 verification via SHIM_LOCK in
> > > efi_multiboot2().
> > >
> > > There appears that some work will be needed on top of this patchset, but 
> > > not
> > > much as it seems most of the foot work has been done.
> > >
> > > AFAIK, the changes needed in GRUB for this approach are already upstream 
> > > (the
> > > shim_lock module is upstream), and shim would go untouched.
> > >
> > > # Option #2: Extended Multiboot2 and Shim
> > >
> > > Another approach that could be taken is to embed Xen's signature into a
> > > new multiboot2 header and then modify shim to support it.  This would
> > > arguably be more readable than embedding the PE/COFF header, would add
> > > value to shim, and would fit nicely with the mb2 header code that
> > > already exists in Xen.  The downside being that it would require a shim
> > > fork.  Grub2 would be unchanged.
>
> Here you have to change the Multiboot2 protocol and singing tools too.
> So, I do not consider this as a viable solution.
>
> > > I'm not familliar with Microsoft's signing process.  I do know they
> > > support template submissions based on shim, and I'm not sure if such a
> > > big change would impact their approval process.
> > >
> > > # Option #3: Lean on Grub2's LoadFile2() Verification
> > >
> > > Grub2 will provide a LoadFile2() method to subsequent programs that 
> > > supports
> > > signature verification of arbitrary files.  Linux is moving in the
> > > direction of using LoadFile2() for loading the initrd [2], and Grub2 will
> > > support verifying the signatures of files loaded via LoadFile2().  This 
> > > is set
> > > for release in GRUB 2.06 sometime in the latter half of 2020.
>
> s/for release in/after release/
>
> > > In Xen, this approach could be used for loading dom0 as well, offering a 
> > > very
> > > simple verified load interface.
> > >
> > > Currently the initrd argument passed from Linux to LoadFile2() is a vendor
> > > media device path GUID [3].
> > >
> > > Changes to Xen:
> > > - Xen would need to pass these device paths to Grub
> > > - Xen would be changed to load dom0 with the LoadFile2() interface via 
> > > boot services
> > >
> > > Changes to Grub:
> > > - Xen dom0 kernel/initrd device paths would need to be introduced to Grub
>
> Maybe partially but certainly there will be some differences...
>
> > > Potential Issues:
> > > - How will Xen handle more boot modules than just a dom0 and dom0
> > >   initrd?
> > > - Would each boot module need a unique vendor guid?
>
> AIUI yes but Ard, who designed this new boot protocol, may say more
> about that.
>
> Anyway, the advantage of this new protocol is that it uses UEFI API to
> load and execute PE kernel and its modules. This means that it will be
> architecture independent. However, IIRC, if we want to add new modules
> types to the Xen then we have to teach all bootloaders in the wild about
> new GUIDs. Ard, am I correct?
>

Well, if you are passing multiple files that are not interchangeable,
each bootloader will need to do something, right? It could be another
GUID, or we could extend the initrd media device path to take
discriminators.

So today, we have

VendorMedia(5568e427-68fc-4f3d-ac74-ca555231cc68)

which identifies /the/ initrd on Linux. As long as this keeps working
as intended, I have no objections extending this to

VendorMedia(5568e427-68fc-4f3d-ac74-ca555231cc68)/File(...)

etc, if we can agree that omitting the File() part means the unnamed
initrd, and that L"initrd" is reserved as a file name. That way, you
can choose any abstract file name you want, but please note that the
loader still needs to provide some kind of mapping of how these
abstract file names relate to actual files selected by the user.

One thing to keep in mind, though, is that this 

Re: [edk2-devel] [PATCH v2 02/31] OvmfPkg: Create platform XenOvmf

[Ard Biesheuvel]

On Wed, 10 Apr 2019 at 07:27, Laszlo Ersek  wrote:
>
> On 04/10/19 11:48, Jordan Justen wrote:
> > On 2019-04-09 04:08:15, Anthony PERARD wrote:
> >> This is a copy of OvmfX64, removing VirtIO and some SMM.
> >>
> >> This new platform will be changed to make it works on two types of Xen
> >> guest: HVM and PVH.
> >>
> >> Compare to OvmfX64, this patch:
> >>
> >> - changed: PLATFORM_GUID, OUTPUT_DIRECTORY, FLASH_DEFINITION
> >> - removed: VirtioLib class resolution
> >> - removed: all UEFI_DRIVER modules for virtio devices
> >> - removed: DXE_SMM_DRIVER and SMM_CORE lib class resolutions
> >> - removed: DXE_SMM_DRIVER and SMM_CORE FDF rules
> >> - removed: Everything related to SMM_REQUIRE==true
> >> - removed: Everything related to SECURE_BOOT_ENABLE==true
> >> - removed: Everything related to TPM2_ENABLE==true
> >> - changed: PcdPciDisableBusEnumeration dynamic default flipped to TRUE
> >> - changed: default FD_SIZE_IN_KB to 2M.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Anthony PERARD 
> >> ---
> >>  OvmfPkg/{OvmfPkgX64.dsc => XenOvmf.dsc} | 202 +---
> >
> > I guess we all want our name to be first :), but OvmfXen seems more
> > like the pattern that edk2 uses when making sub-platforms.
> >
> > Also, in some cases we've used !ifdef type things to keep dsc and fdf
> > files common. Would that not be workable here? Maybe it would get too
> > ugly. :\
>
> I've been happy with a similar Xen<->QEMU split under ArmVirtPkg.
> Duplications in updates are usually not a huge burden, and keeping the
> files separate has proved very helpful for determining
> maintainer/reviewer/tester responsibility.
>
> > It could help to prevent having to sync dsc changes across, and
> > prevent changes from being omitted for Xen on accident.
>
> True, but in my experience that's been the smaller problem. The larger
> problem has been modifying Xen stuff in unintended ways (= regressing
> Xen), because we can't test (or don't even notice) the Xen-side
> implications of changes made to common source.
>
> Personally I prefer another (DSC + FDF) pair under OvmfPkg (beyond the
> three we already have) to another layer of (conditional?) includes. The
> !if's that are being eliminated in this Xen-customized copy (i.e., in
> this patch) are complex enough already :)
>
> ... It's not that I *generally* prefer duplication; as you say, we do
> have a number of !includes already. I do prefer duplication specifically
> for Xen vs. QEMU however.
>

+1

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38825): https://edk2.groups.io/g/devel/message/38825
Mute This Topic: https://groups.io/mt/30997887/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] OvmfPkg: replace MIT license blocks with SPDX IDs

[Ard Biesheuvel]

On Wed, 10 Apr 2019 at 05:58, Laszlo Ersek  wrote:
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: ovmf_spdx_mit
>
> For <https://bugzilla.tianocore.org/show_bug.cgi?id=1373>, we replaced
> open-coded license text blocks with "SPDX-License-Identifier"s, almost
> all over the edk2 tree.
>
> That change however was tied to a license update / CLA update too
> (please see details in TianoCore#1373, referenced above), and therefore
> the MIT-covered files under OvmfPkg were out of scope. We filed a
> reminder at <https://bugzilla.tianocore.org/show_bug.cgi?id=1654>, and
> this series intends to address that -- i.e., to replace the MIT license
> blocks with the corresponding SPDX License IDs.
>
> Cc: Anthony Perard 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Julien Grall 
> Cc: Lars Kurth 
> Cc: xen-devel@lists.xenproject.org
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (4):
>   OvmfPkg/License.txt: remove XenPvBlkDxe from the MIT licensed dir list
>   OvmfPkg/License.txt: refresh the MIT license text and include the SPDX
> ID
>   OvmfPkg/IndustryStandard/Xen: replace MIT license text with SPDX ID
>   OvmfPkg/XenBusDxe: replace MIT license text with SPDX ID
>

For the series

Reviewed-by: Ard Biesheuvel 

>  OvmfPkg/Include/IndustryStandard/Xen/arch-arm/xen.h| 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen-x86_32.h | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen-x86_64.h | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/arch-x86/xen.h| 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/event_channel.h   | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/grant_table.h | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/hvm/hvm_op.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/hvm/params.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/blkif.h| 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/console.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/protocols.h| 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/ring.h | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/xenbus.h   | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/io/xs_wire.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/memory.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/xen-compat.h  | 18 
> +-
>  OvmfPkg/Include/IndustryStandard/Xen/xen.h | 18 
> +-
>  OvmfPkg/License.txt|  8 +---
>  OvmfPkg/XenBusDxe/XenBus.c | 18 
> +-
>  OvmfPkg/XenBusDxe/XenStore.c   | 18 
> +-
>  OvmfPkg/XenBusDxe/XenStore.h   | 18 
> +-
>  21 files changed, 25 insertions(+), 343 deletions(-)
>
> --
> 2.19.1.3.g30247aa5d201
>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#38823): https://edk2.groups.io/g/devel/message/38823
Mute This Topic: https://groups.io/mt/31018509/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [Xen-devel] [PATCH 1/3] treewide: Lift switch variables out of switches

[Ard Biesheuvel]

On Wed, 23 Jan 2019 at 13:09, Jann Horn  wrote:
>
> On Wed, Jan 23, 2019 at 1:04 PM Greg KH  wrote:
> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > > Variables declared in a switch statement before any case statements
> > > cannot be initialized, so move all instances out of the switches.
> > > After this, future always-initialized stack variables will work
> > > and not throw warnings like this:
> > >
> > > fs/fcntl.c: In function ‘send_sigio_to_task’:
> > > fs/fcntl.c:738:13: warning: statement will never be executed 
> > > [-Wswitch-unreachable]
> > >siginfo_t si;
> > >  ^~
> >
> > That's a pain, so this means we can't have any new variables in { }
> > scope except for at the top of a function?
>
> AFAICS this only applies to switch statements (because they jump to a
> case and don't execute stuff at the start of the block), not blocks
> after if/while/... .
>

I guess that means it may apply to other cases where you do a 'goto'
into the middle of a for() loop, for instance (at the first
iteration), which is also a valid pattern.

Is there any way to tag these assignments so the diagnostic disregards them?

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Unable to boot Linux with master EDK2

[Ard Biesheuvel]

On Fri, 18 Jan 2019 at 19:39, Ard Biesheuvel  wrote:
>
> On Fri, 18 Jan 2019 at 19:30, Julien Grall  wrote:
> >
> > Hi all,
> >
> > I am trying to boot a Xen guest using the latest EDK2 master (cce9d76358
> > "BaseTools: Allow empty value for HiiPcd in Dsc"), GRUB and Linux 5.0-rc2.
> >
> > The last code executed by Linux is when installing the virtual address
> > map in the EFI stub and then it seems to get stuck. I don't have much
> > information from the console:
> >
> > InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 7E041040
> > Loading driver at 0x00068C7 EntryPoint=0x00069D65664
> > Loading driver at 0x00068C7 EntryPoint=0x00069D65664
> > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7DF6AB18
> > ProtectUefiImageCommon - 0x7E041040
> >   - 0x68C7 - 0x02006000
> > SetUefiImageMemoryAttributes - 0x68C7 - 0x1000 
> > (0x4008)
> > SetUefiImageMemoryAttributes - 0x68C71000 - 0x011CD000 
> > (0x00020008)
> > SetUefiImageMemoryAttributes - 0x69E3E000 - 0x00E38000 
> > (0x4008)
> > EFI stub: Booting Linux Kernel...
> > EFI stub: Using DTB from configuration table
> > EFI stub: Exiting boot services and installing virtual address map...
> > XenBus: Set state to 5
> > XenBus: Set state to 5, done
> > XenPvBlk: waiting backend state 5, current: 4
> > XenStore: Watch event 7E957398
> > XenBus: Set state to 6
> > XenBus: Set state to 6, done
> > XenPvBlk: waiting backend state 6, current: 5
> > XenStore: Watch event 7E957398
> > XenBus: Set state to 1
> > XenBus: Set state to 1, done
> > Xen GrantTable, removing 38003
> > Xen GrantTable, removing 38002
> > Xen GrantTable, removing 38001
> > Xen GrantTable, removing 38000
> > SetUefiImageMemoryAttributes - 0x7F36 - 0x0004 
> > (0x0008)
> > SetUefiImageMemoryAttributes - 0x7BFF - 0x0004 
> > (0x0008)
> > SetUefiImageMemoryAttributes - 0x7BFA - 0x0004 
> > (0x0008)
> > SetUefiImageMemoryAttributes - 0x7BF0 - 0x0004 
> > (0x0008)
> > SetUefiImageMemoryAttributes - 0x7BE6 - 0x0004 
> > (0x0008)
> > SetUefiImageMemoryAttributes - 0x7BDC - 0x0004 
> > (0x0008)
> >
> > The bisector pointed to the following commit:
> >
> > commit 2f4a5a9f4c17ed88aaa3114d1e161e42cb80a9bf
> > Author: Dandan Bi 
> > Date:   Thu Jan 3 15:31:23 2019 +0800
> >
> > MdePkg/BasePeCoffLib: Add more check for relocation data
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1426
> >
> > V2:
> > (1) Add NULL pointer check for the input parameters
> > (2) Add check for the "Adjust" value before applying fix ups.
> >
> > In function PeCoffLoaderRelocateImageForRuntime, it doesn't
> > do much check when do relocation. For API level consideration,
> > it's not safe enough.
> > So this patch is to replace the same code logic with function
> > PeCoffLoaderImageAddress which will cover more validation.
> >
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Dandan Bi 
> > Reviewed-by: Liming Gao 
> >
> > Any ideas what could have gone wrong?
> >

Yes, that patch broke lots of platforms: OVMF, ArmVirtQemu and ARM
Juno as well. You need the following patch to fix it

https://lists.01.org/pipermail/edk2-devel/2019-January/035372.html

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Unable to boot Linux with master EDK2

[Ard Biesheuvel]

On Fri, 18 Jan 2019 at 19:30, Julien Grall  wrote:
>
> Hi all,
>
> I am trying to boot a Xen guest using the latest EDK2 master (cce9d76358
> "BaseTools: Allow empty value for HiiPcd in Dsc"), GRUB and Linux 5.0-rc2.
>
> The last code executed by Linux is when installing the virtual address
> map in the EFI stub and then it seems to get stuck. I don't have much
> information from the console:
>
> InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 7E041040
> Loading driver at 0x00068C7 EntryPoint=0x00069D65664
> Loading driver at 0x00068C7 EntryPoint=0x00069D65664
> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7DF6AB18
> ProtectUefiImageCommon - 0x7E041040
>   - 0x68C7 - 0x02006000
> SetUefiImageMemoryAttributes - 0x68C7 - 0x1000 
> (0x4008)
> SetUefiImageMemoryAttributes - 0x68C71000 - 0x011CD000 
> (0x00020008)
> SetUefiImageMemoryAttributes - 0x69E3E000 - 0x00E38000 
> (0x4008)
> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
> XenBus: Set state to 5
> XenBus: Set state to 5, done
> XenPvBlk: waiting backend state 5, current: 4
> XenStore: Watch event 7E957398
> XenBus: Set state to 6
> XenBus: Set state to 6, done
> XenPvBlk: waiting backend state 6, current: 5
> XenStore: Watch event 7E957398
> XenBus: Set state to 1
> XenBus: Set state to 1, done
> Xen GrantTable, removing 38003
> Xen GrantTable, removing 38002
> Xen GrantTable, removing 38001
> Xen GrantTable, removing 38000
> SetUefiImageMemoryAttributes - 0x7F36 - 0x0004 
> (0x0008)
> SetUefiImageMemoryAttributes - 0x7BFF - 0x0004 
> (0x0008)
> SetUefiImageMemoryAttributes - 0x7BFA - 0x0004 
> (0x0008)
> SetUefiImageMemoryAttributes - 0x7BF0 - 0x0004 
> (0x0008)
> SetUefiImageMemoryAttributes - 0x7BE6 - 0x0004 
> (0x0008)
> SetUefiImageMemoryAttributes - 0x7BDC - 0x0004 
> (0x0008)
>
> The bisector pointed to the following commit:
>
> commit 2f4a5a9f4c17ed88aaa3114d1e161e42cb80a9bf
> Author: Dandan Bi 
> Date:   Thu Jan 3 15:31:23 2019 +0800
>
> MdePkg/BasePeCoffLib: Add more check for relocation data
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1426
>
> V2:
> (1) Add NULL pointer check for the input parameters
> (2) Add check for the "Adjust" value before applying fix ups.
>
> In function PeCoffLoaderRelocateImageForRuntime, it doesn't
> do much check when do relocation. For API level consideration,
> it's not safe enough.
> So this patch is to replace the same code logic with function
> PeCoffLoaderImageAddress which will cover more validation.
>
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi 
> Reviewed-by: Liming Gao 
>
> Any ideas what could have gone wrong?
>
> Best regards,
>
> --
> Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Ard Biesheuvel]

On 11 April 2018 at 10:56, Daniel Kiper  wrote:
> On Wed, Apr 04, 2018 at 12:38:24PM +0200, Daniel Kiper wrote:
>> On Tue, Apr 03, 2018 at 10:00:52AM -0700, James Bottomley wrote:
>> > On Tue, 2018-04-03 at 18:07 +0200, Daniel Kiper wrote:
>> > > On Tue, Apr 03, 2018 at 08:44:41AM -0700, James Bottomley wrote:
>>
>> [...]
>>
>> > > > This looks like a bad idea: you're duplicating the secure boot
>> > > > check in
>> > > >
>> > > > drivers/firmware/efi/libstub/secureboot.c
>> > > >
>> > > > Which is an implementation of policy.  If we have to have policy in
>> > > > the kernel, it should really only be in one place to prevent drift;
>> > > > why can't you simply use the libstub efi_get_secureboot() so we're
>> > > > not duplicating the implementation of policy?
>> > >
>> > > Well, here is the first version of this patch:
>> > > https://lkml.org/lkml/2018/1/9/496 Ard did not like it. I was not
>> > > happy too. In general both approaches are not perfect. More you can
>> > > find in the discussion around this patchset. If you have better idea
>> > > how to do that I am happy to implement it.
>> >
>> > One way might be simply to have the pre exit-boot-services code lay
>> > down a variable containing the state which you pick up, rather than you
>>
>> Do you mean variable in kernel proper or something like that? If yes this
>> is not possible. EFI Linux stub is not executed in Xen dom0. All UEFI
>> infrastructure is owned and operated by Xen. Dom0 kernel can access some
>> stuff in UEFI, including variables, via hypercall. However, when dom0
>> runs only UEFI runtime services are available.
>>
>> > calling efi code separately and trying to use the insecure RT
>>
>> I am not sure why they are insecure.
>>
>> > variables.  That way there's a uniform view of the internal kernel
>> > secure boot state that everyone can use.
>>
>> That would be perfect but I have a feeling that in form proposed above
>> it is not possible.
>
> Ping?
>

(apologies if this is a duplicate email - I thought I had replied
already but I don't see it in my sent folder)

Queued in efi/next - thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Ard Biesheuvel]

On 12 January 2018 at 11:24, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> Hi Ard,
>
> On Thu, Jan 11, 2018 at 12:51:07PM +, Ard Biesheuvel wrote:
>> On 9 January 2018 at 14:22, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>> > Hi,
>> >
>> > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
>> > may not even know that it runs on secure boot enabled platform.
>>
>> Hi Daniel,
>>
>> I must say, I am not too thrilled with the approach you have chosen
>> here. #including .c files in other .c files, and using #defines to
>> override C functions or other stub functionality is rather fragile. In
>
> TBH I do not like it too. Sadly I have not find a better solution for
> that. I wish to avoid code duplication as much as possible because
> otherwise it will fall out of sync sooner or later (usually sooner).
> Similar thing happened in different part of Xen EFI code a few months ago.
>
>> particular, it means we have to start caring about not breaking
>> Xen/x86 code when making modifications to the EFI stub, and that code
>> is already difficult enough to maintain, given that it is shared
>> between ARM, arm64 and x86, and runs either from the decompressor or
>> the kernel proper (arm64) but in the context of the UEFI firmware.
>
> I understand that.
>
>> None of the stub code currently runs in ordinary kernel context.
>
> Yep.
>
>> So please, could you try to find another way to do this?
>
> I am happy to improve the situation, however, I am afraid that it is
> difficult here. Stub and kernel proper are separate entities and simple
> linking does not work. So, It seems to me that only play with includes
> will allow us to not duplicate the code. However, if you have a better
> idea I am happy to implement it.
>

Actually, there is another reason why it does not make sense to reuse that code.

This code

/*
* See if a user has put the shim into insecure mode. If so, and if the
* variable doesn't have the runtime attribute set, we might as well
* honor that.
*/
size = sizeof(moksbstate);
status = get_efi_var(L"MokSBState", _guid,
 , , );

/* If it fails, we don't care why. Default to secure */
if (status != EFI_SUCCESS)
goto secure_boot_enabled;
if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
return efi_secureboot_mode_disabled;

will always fail after exiting boot services, so it makes no sense to
call it from xen_efi_init().

So I suggest you just clone the function and only keep the pieces that
make sense for Xen.

-- 
Ard.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Ard Biesheuvel]

On 9 January 2018 at 14:22, Daniel Kiper  wrote:
> Hi,
>
> Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
> may not even know that it runs on secure boot enabled platform.
>

Hi Daniel,

I must say, I am not too thrilled with the approach you have chosen
here. #including .c files in other .c files, and using #defines to
override C functions or other stub functionality is rather fragile. In
particular, it means we have to start caring about not breaking
Xen/x86 code when making modifications to the EFI stub, and that code
is already difficult enough to maintain, given that it is shared
between ARM, arm64 and x86, and runs either from the decompressor or
the kernel proper (arm64) but in the context of the UEFI firmware.
None of the stub code currently runs in ordinary kernel context.

So please, could you try to find another way to do this?

Thanks,
Ard.


>
>  arch/x86/xen/Makefile  |4 +++-
>  arch/x86/xen/efi.c |   14 +
>  drivers/firmware/efi/libstub/secureboot-core.c |   77 
> +
>  drivers/firmware/efi/libstub/secureboot.c  |   66 
> +--
>  4 files changed, 99 insertions(+), 62 deletions(-)
>
> Daniel Kiper (4):
>   efi/stub: Extract efi_get_secureboot() to separate file
>   x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init()
>   efi: Tweak efi_get_secureboot() and its data section assignment
>   efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it 
> static
>

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel