| Message ID | 1494260238-30713-1-git-send-email-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/05/17 17:17, Ross Lagerwall wrote: > Some EFI firmware implementations may place the EFI properties table in > RAM marked as BootServicesData, which Xen does not consider as reserved. > When dom0 tries to access the EFI properties table (which Linux >= 4.4 > does), it crashes with a page fault. The pagefault is just a side effect of Linux blindly assuming that the ioremap() request succeeded. From Xen's point of view, Dom0 tries to map a page which doesn't belong to dom_xen, resulting a permission failure. > Fix this by unconditionally > marking the EFI properties table as reserved in the E820, much like is > done with the dmi regions. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This is probably also 4.9 material.
On Mon, May 08, 2017 at 05:17:18PM +0100, Ross Lagerwall wrote: > Some EFI firmware implementations may place the EFI properties table in > RAM marked as BootServicesData, which Xen does not consider as reserved. > When dom0 tries to access the EFI properties table (which Linux >= 4.4 > does), it crashes with a page fault. Fix this by unconditionally > marking the EFI properties table as reserved in the E820, much like is > done with the dmi regions. Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> And also found who sets this: https://github.com/tianocore/edk2/blob/1860cb00c18c6f0c58336ea15a63889dabd31d15/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c Don't know if you want to include that in the blurb so somebody can also look up why it is being set. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > --- > xen/arch/x86/efi/efi-boot.h | 8 ++++++++ > xen/common/efi/boot.c | 20 ++++++++++++++++++++ > xen/common/efi/efi.h | 4 ++++ > 3 files changed, 32 insertions(+) > > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 34537d4..36ae464 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -209,6 +209,14 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > } > } > > + if ( efi_properties_tbl_addr && efi_properties_tbl_size ) > + { > + ++e; > + e->addr = efi_properties_tbl_addr; > + e->size = efi_properties_tbl_size; > + e->type = E820_RESERVED; > + ++e820_raw.nr_map; > + } > } > > static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index daf0c80..1fb396b 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -40,6 +40,8 @@ > { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} } > #define APPLE_PROPERTIES_PROTOCOL_GUID \ > { 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} } > +#define EFI_PROPERTIES_TABLE_GUID \ > + { 0x880aaca3, 0x4adc, 0x4a04, { 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5} } > > typedef EFI_STATUS > (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) ( > @@ -171,6 +173,15 @@ static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) > ebmalloc_mem[EBMALLOC_SIZE]; > static unsigned long __initdata ebmalloc_allocated; > > +struct efi_properties_table { > + u32 version; > + u32 length; > + u64 memory_protection_attribute; > +}; > + > +u64 __initdata efi_properties_tbl_addr; > +u32 __initdata efi_properties_tbl_size; > + > /* EFI boot allocator. */ > static void __init __maybe_unused *ebmalloc(size_t size) > { > @@ -809,6 +820,7 @@ static void __init efi_tables(void) > static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; > static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; > static EFI_GUID __initdata smbios3_guid = SMBIOS3_TABLE_GUID; > + static EFI_GUID __initdata properties_guid = EFI_PROPERTIES_TABLE_GUID; > > if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) > efi.acpi20 = (long)efi_ct[i].VendorTable; > @@ -820,6 +832,14 @@ static void __init efi_tables(void) > efi.smbios = (long)efi_ct[i].VendorTable; > if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) ) > efi.smbios3 = (long)efi_ct[i].VendorTable; > + if ( match_guid(&properties_guid, &efi_ct[i].VendorGuid) ) > + { > + struct efi_properties_table *properties; > + > + efi_properties_tbl_addr = (long)efi_ct[i].VendorTable; > + properties = (struct efi_properties_table *)efi_properties_tbl_addr; > + efi_properties_tbl_size = properties->length; > + } > } > > #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */ > diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h > index 6b9c56e..e509111 100644 > --- a/xen/common/efi/efi.h > +++ b/xen/common/efi/efi.h > @@ -5,6 +5,7 @@ > #include <efi/efidevp.h> > #include <efi/eficapsule.h> > #include <efi/efiapi.h> > +#include <xen/init.h> > #include <xen/efi.h> > #include <xen/spinlock.h> > #include <asm/page.h> > @@ -39,3 +40,6 @@ extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size, > > extern UINT64 efi_apple_properties_addr; > extern UINTN efi_apple_properties_len; > + > +extern u64 __initdata efi_properties_tbl_addr; > +extern u32 __initdata efi_properties_tbl_size; > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 08.05.17 at 18:17, <ross.lagerwall@citrix.com> wrote: > Some EFI firmware implementations may place the EFI properties table in > RAM marked as BootServicesData, which Xen does not consider as reserved. And which is correct. Hence ... > When dom0 tries to access the EFI properties table (which Linux >= 4.4 > does), it crashes with a page fault. Fix this by unconditionally > marking the EFI properties table as reserved in the E820, ... "fix this by" is the wrong term, "work around this by" would be more suitable. But what's worse - Linux has no business looking at the sole bit defined in MemoryProtectionAttribute, as that's relevant to Xen (as the exclusive entity dealing with the machine memory map) only. While I view this by itself as a reason to NAK this patch, I'll nevertheless give a few comments below. > much like is done with the dmi regions. ??? efi_arch_process_memory_map() doesn't have any DMI specific code, and you don't even come close to introducing behavior similar to dmi_efi_get_table() / dmi_get_table(), which results in a proper call to reserve_e820_ram() as opposed to ... > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -209,6 +209,14 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > } > } > > + if ( efi_properties_tbl_addr && efi_properties_tbl_size ) > + { > + ++e; > + e->addr = efi_properties_tbl_addr; > + e->size = efi_properties_tbl_size; > + e->type = E820_RESERVED; > + ++e820_raw.nr_map; > + } ... you creating an (in the case you want to deal with) overlapping entry, which we should really avoid. > @@ -171,6 +173,15 @@ static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) > ebmalloc_mem[EBMALLOC_SIZE]; > static unsigned long __initdata ebmalloc_allocated; > > +struct efi_properties_table { > + u32 version; > + u32 length; > + u64 memory_protection_attribute; > +}; > + > +u64 __initdata efi_properties_tbl_addr; > +u32 __initdata efi_properties_tbl_size; No new uses of u32 / u64 please. Namely considering the patch context of the header change, it should have occurred to you to use UINT64 / UINT32 / UINTN here. Also please use the properly spelled structure tag and field names, as per the UEFI spec. Eventually I'd expect these to appear in one of the canonical EFI headers, at which time it should be possible to simply drop the definition here without the need to adjust any other code. > @@ -820,6 +832,14 @@ static void __init efi_tables(void) > efi.smbios = (long)efi_ct[i].VendorTable; > if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) ) > efi.smbios3 = (long)efi_ct[i].VendorTable; > + if ( match_guid(&properties_guid, &efi_ct[i].VendorGuid) ) > + { > + struct efi_properties_table *properties; const > + efi_properties_tbl_addr = (long)efi_ct[i].VendorTable; > + properties = (struct efi_properties_table *)efi_properties_tbl_addr; This second cast could be easily avoided if you assigned from efi_ct[i].VendorTable, which is VOID *. > @@ -39,3 +40,6 @@ extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size, > > extern UINT64 efi_apple_properties_addr; > extern UINTN efi_apple_properties_len; > + > +extern u64 __initdata efi_properties_tbl_addr; > +extern u32 __initdata efi_properties_tbl_size; __initdata does not belong onto declarations. Only for definitions this annotation is meaningful. Jan
Hi Andrew, On 08/05/17 17:29, Andrew Cooper wrote: > On 08/05/17 17:17, Ross Lagerwall wrote: >> Some EFI firmware implementations may place the EFI properties table in >> RAM marked as BootServicesData, which Xen does not consider as reserved. >> When dom0 tries to access the EFI properties table (which Linux >= 4.4 >> does), it crashes with a page fault. > > The pagefault is just a side effect of Linux blindly assuming that the > ioremap() request succeeded. > > From Xen's point of view, Dom0 tries to map a page which doesn't belong > to dom_xen, resulting a permission failure. > >> Fix this by unconditionally >> marking the EFI properties table as reserved in the E820, much like is >> done with the dmi regions. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > This is probably also 4.9 material. It looks like Jan had some comments on this patch. I haven't seen any reply from Ross, I will wait before looking from a release perspective. Cheers,
On 05/15/2017 02:52 PM, Julien Grall wrote: > Hi Andrew, > > On 08/05/17 17:29, Andrew Cooper wrote: >> On 08/05/17 17:17, Ross Lagerwall wrote: >>> Some EFI firmware implementations may place the EFI properties table in >>> RAM marked as BootServicesData, which Xen does not consider as reserved. >>> When dom0 tries to access the EFI properties table (which Linux >= 4.4 >>> does), it crashes with a page fault. >> >> The pagefault is just a side effect of Linux blindly assuming that the >> ioremap() request succeeded. >> >> From Xen's point of view, Dom0 tries to map a page which doesn't belong >> to dom_xen, resulting a permission failure. >> >>> Fix this by unconditionally >>> marking the EFI properties table as reserved in the E820, much like is >>> done with the dmi regions. >>> >>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> This is probably also 4.9 material. > > It looks like Jan had some comments on this patch. I haven't seen any > reply from Ross, I will wait before looking from a release perspective. > Jan had some comments on that patch but essentially NAKed it on principle so I don't think it will be going into 4.9. Thanks,
>>> On 17.05.17 at 18:21, <ross.lagerwall@citrix.com> wrote: > On 05/15/2017 02:52 PM, Julien Grall wrote: >> Hi Andrew, >> >> On 08/05/17 17:29, Andrew Cooper wrote: >>> On 08/05/17 17:17, Ross Lagerwall wrote: >>>> Some EFI firmware implementations may place the EFI properties table in >>>> RAM marked as BootServicesData, which Xen does not consider as reserved. >>>> When dom0 tries to access the EFI properties table (which Linux >= 4.4 >>>> does), it crashes with a page fault. >>> >>> The pagefault is just a side effect of Linux blindly assuming that the >>> ioremap() request succeeded. >>> >>> From Xen's point of view, Dom0 tries to map a page which doesn't belong >>> to dom_xen, resulting a permission failure. >>> >>>> Fix this by unconditionally >>>> marking the EFI properties table as reserved in the E820, much like is >>>> done with the dmi regions. >>>> >>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >>> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> This is probably also 4.9 material. >> >> It looks like Jan had some comments on this patch. I haven't seen any >> reply from Ross, I will wait before looking from a release perspective. > > Jan had some comments on that patch but essentially NAKed it on > principle so I don't think it will be going into 4.9. Let me clarify the NAK a little: Besides there having been no opposition to it (indicating to me that the grounds on which I gave it are acceptable), I'd revisit this if a serious attempt was made to make Linux behave sanely here, and that attempt was (bogusly) rejected. After all Linux is already prepared to deal with there not being any EFI memory map, so it would seem pretty strange to me if they didn't accept a (presumably small) change to skip retrieving the properties table in that case (and perhaps at once the memory attributes one). (And btw, I seem to recall indication that the Dom0 boot problem was due to an unchecked ioremap() or alike call, but afaict 4.11 does have a check there, so I'm currently at a loss to see where the boot failure would come from.) Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 34537d4..36ae464 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -209,6 +209,14 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, } } + if ( efi_properties_tbl_addr && efi_properties_tbl_size ) + { + ++e; + e->addr = efi_properties_tbl_addr; + e->size = efi_properties_tbl_size; + e->type = E820_RESERVED; + ++e820_raw.nr_map; + } } static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index daf0c80..1fb396b 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -40,6 +40,8 @@ { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} } #define APPLE_PROPERTIES_PROTOCOL_GUID \ { 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} } +#define EFI_PROPERTIES_TABLE_GUID \ + { 0x880aaca3, 0x4adc, 0x4a04, { 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5} } typedef EFI_STATUS (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) ( @@ -171,6 +173,15 @@ static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) ebmalloc_mem[EBMALLOC_SIZE]; static unsigned long __initdata ebmalloc_allocated; +struct efi_properties_table { + u32 version; + u32 length; + u64 memory_protection_attribute; +}; + +u64 __initdata efi_properties_tbl_addr; +u32 __initdata efi_properties_tbl_size; + /* EFI boot allocator. */ static void __init __maybe_unused *ebmalloc(size_t size) { @@ -809,6 +820,7 @@ static void __init efi_tables(void) static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; static EFI_GUID __initdata smbios3_guid = SMBIOS3_TABLE_GUID; + static EFI_GUID __initdata properties_guid = EFI_PROPERTIES_TABLE_GUID; if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) efi.acpi20 = (long)efi_ct[i].VendorTable; @@ -820,6 +832,14 @@ static void __init efi_tables(void) efi.smbios = (long)efi_ct[i].VendorTable; if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) ) efi.smbios3 = (long)efi_ct[i].VendorTable; + if ( match_guid(&properties_guid, &efi_ct[i].VendorGuid) ) + { + struct efi_properties_table *properties; + + efi_properties_tbl_addr = (long)efi_ct[i].VendorTable; + properties = (struct efi_properties_table *)efi_properties_tbl_addr; + efi_properties_tbl_size = properties->length; + } } #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */ diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h index 6b9c56e..e509111 100644 --- a/xen/common/efi/efi.h +++ b/xen/common/efi/efi.h @@ -5,6 +5,7 @@ #include <efi/efidevp.h> #include <efi/eficapsule.h> #include <efi/efiapi.h> +#include <xen/init.h> #include <xen/efi.h> #include <xen/spinlock.h> #include <asm/page.h> @@ -39,3 +40,6 @@ extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size, extern UINT64 efi_apple_properties_addr; extern UINTN efi_apple_properties_len; + +extern u64 __initdata efi_properties_tbl_addr; +extern u32 __initdata efi_properties_tbl_size;
Some EFI firmware implementations may place the EFI properties table in RAM marked as BootServicesData, which Xen does not consider as reserved. When dom0 tries to access the EFI properties table (which Linux >= 4.4 does), it crashes with a page fault. Fix this by unconditionally marking the EFI properties table as reserved in the E820, much like is done with the dmi regions. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/x86/efi/efi-boot.h | 8 ++++++++ xen/common/efi/boot.c | 20 ++++++++++++++++++++ xen/common/efi/efi.h | 4 ++++ 3 files changed, 32 insertions(+)