On Fri, 30 Sept 2022 at 20:17, Demi Marie Obenour
<demi@xxxxxxxxxxxxxxxxxxxxxx> 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
> > <demi@xxxxxxxxxxxxxxxxxxxxxx> 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 <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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 = &config_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 disregarded, but also (if the table should not be
> ignored) the end of the memory region containing it.
But the calling code never uses that value, right?
> I will make
> xen_efi_config_table_valid() a wrapper around
> xen_config_table_memory_region_max(), as the former also needs to print
> a warning if the table is in an invalid location.
>
> > 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.
>
> What should I use instead?
>
> > > + 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, &efi.flags);
> > >
> > > 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 <xen/interface/platform.h>
> > > #include <xen/xen.h>
> > > #include <xen/xen-ops.h>
> > > +#include <xen/page.h>
> > >
> > > #include <asm/page.h>
> > >
> > > @@ -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.
>
> Will fix in v5.
>
> > > +{
> > > + 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?
>
> My understanding is that it should never happen on any architecture.
EFI_PAGE_SHIFT is always 12, on any architecture and regardless of the
page size used by the OS itself. AFAICT, the same applies to
XEN_PAGE_SHIFT.
> That’s why I static_assert() it. I have no idea if this is the only
> place it matters, though.
>
I don't mind adding this here, but it's kind of orthogonal to the rest
of the patch so please make a mention in the commit log why you are
adding it.
> > > + 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 xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > + int rc = HYPERVISOR_platform_op(&op);
> > > +
> > > + if (rc) {
> > > + pr_warn("Failed to lookup header %llu in Xen memory map: error %d\n",
> > > + (unsigned long long)addr, rc);
> > > + return 0;
> > > + }
> > > +
> > > + switch (info->mem.type) {
> > > + case EFI_RUNTIME_SERVICES_CODE:
> > > + case EFI_RUNTIME_SERVICES_DATA:
> > > + case EFI_ACPI_RECLAIM_MEMORY:
> >
> > If we are listing all memory types that Xen preserves, you might add
> > EFI_RESERVED_MEMORY here. Otherwise, please only list the ones that
> > you need to permit explicitly.
>
> My understanding was that EFI_RESERVED_MEMORY should never be touched by
> the OS, so I left it out. Which types would you permit?
>
Well, given the purpose of the function (i.e, to reject
EfiBootServicesData in spite of the spec), I'd only permit
EFI_ACPI_RECLAIM_MEMORY and EFI_RUNTIME_SERVICES_DATA. However, the
EFI spec does mention that prior versions permitted the reserved
memory type as well for ACPI and SMBIOS tables (although that may be a
very long time ago).
Bottom line is that you want to permit only regions that Xen is
guaranteed not to clobber, right? In any case, I'm not going to obsess
over this so if you prefer to keep it this way, that's also fine.
> > > + return info->mem.addr + info->mem.size;
> > > + default:
> > > + pr_warn("Table %llu is in memory of type %d, ignoring it\n",
> > > + (unsigned long long)addr, info->mem.type);
> > > + return 0;
> > > + }
> > > +}
> > > +
> > > /*
> > > * Set XEN EFI runtime services function pointers. Other fields of struct efi,
> > > * e.g. efi.systab, will be set like normal EFI.
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index d2b84c2fec39f0268324d1a38a73ed67786973c9..fc81e4b984398cdb399e7886b2cae7f33bf91613 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -1324,4 +1324,13 @@ struct linux_efi_coco_secret_area {
> > > /* 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)
> > >
> > > +#ifdef CONFIG_XEN_EFI
> >
> > Please drop this #ifdef
>
> Will fix in v5.
>
> > > +/*
> > > + * Returns the end of the memory region containing the given config table,
> > > + * or 0 if the given address does not reside in memory that can validly
> > > + * contain EFI configuration tables.
> > > + */
> > > +__init u64 xen_config_table_memory_region_max(u64 addr);
> >
> > You can drop the __init here
>
> Will fix in v5.
>
> > > +#endif
> > > +
> > > #endif /* _LINUX_EFI_H */
> > > --
> > > Sincerely,
> > > Demi Marie Obenour (she/her/hers)
> > > Invisible Things Lab
> > >
>
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
