| Message ID | 1501516597.4771.328.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> David Woodhouse <dwmw2@infradead.org> 07/31/17 5:57 PM >>> >There is a first call to efi_arch_relocate_image(0) before the >ExitBootServices() call. However I'm missing something because I can't >see how that call achieves anything *other* than to trigger the fault >we're concerned about. > >There are three types of relocations — PE_BASE_RELOC_ABS, >PE_BASE_RELOC_HIGHLOW, and PE_BASE_RELOC_DIR64. > >The first (ABS) doesn't seem to do anything, ever. And is never emitted >by mkrelocs.c. > >The second (HIGHLOW) does nothing if (!delta). > >The third (DIR64) simply adds 'delta' to the target address. We could >potentially stop it faulting on that pointless '*addr += 0' by doing >this... > >--- a/xen/arch/x86/efi/efi-boot.h >+++ b/xen/arch/x86/efi/efi-boot.h >@@ -87,7 +87,8 @@ static void __init efi_arch_relocate_image(unsigned long delta) >case PE_BASE_RELOC_DIR64: >if ( in_page_tables(addr) ) >blexit(L"Unexpected relocation type"); >- *(u64 *)addr += delta; >+ if ( delta ) >+ *(u64 *)addr += delta; Yes, that's what I had described as "adjustment to efi_arch_relocate_image()". >break; >default: >blexit(L"Unsupported relocation type"); > > >... but then again, if the whole function is really doing nothing at >all when invoked with delta==0 then perhaps it would just be easier to >remove the first pass altogether. I feel sure I'm missing something, The reason is even visible in context above: We can't call blexit() after having called ExitBootServices(). Hence we need a "dry run" done early, to be certain we won't encounter unhandlable relocations later on. >Either way, this is still broken before my patch though, right? As long as there are .rodata entries needing relocation (which I understand you've found example of), yes. >Especially if UEFI learns to do it for non-executable sections, but >AFAICT even before that. > >These are the sections I see the PE section headers of a local build: > >Name Characteristics Relocations >.text 0xe0d00020 (WRX) ✓ >.rodata 0x40600040 ( R ) ✓ >.buildid 0x40300040 ( R ) >.init.te 0x60500020 ( RX) ✓ >.init.da 0xc0d00040 (WR ) ✓ >.data.re 0xc0800040 (WR ) ✓ >.data 0xc0d00040 (WR ) ✓ >.bss 0xc1000080 (WR ) >.reloc 0x42300040 ( R ) >.pad 0xc0300080 (WR ) > >So there are (again, before my patch) relocations in .init.da(ta) and >.rodata sections which UEFI *might* start marking read-only, and also >in .init.te(xt) which is R+X and could be marked read-only today. Not sure why you mention .init.data, but yes, .init.text could have the same issue. But here it would probably be better to simply mark the section WRX. Jan
On Wed, 2017-08-02 at 03:17 -0600, Jan Beulich wrote: > > >... but then again, if the whole function is really doing nothing at > >all when invoked with delta==0 then perhaps it would just be easier to > >remove the first pass altogether. I feel sure I'm missing something, > > The reason is even visible in context above: We can't call blexit() after > having called ExitBootServices(). Hence we need a "dry run" done early, > to be certain we won't encounter unhandlable relocations later on. Ah, thanks. I'd glossed over that as a "can never happen" condition. And the 'default:' case really is that — the build system is broken if we ever see that. But the DIR64 / in_page_tables() one is less provably impossible. > >Either way, this is still broken before my patch though, right? > > As long as there are .rodata entries needing relocation (which I > understand you've found example of), yes. And more to the point, .init.text entries needing relocation (since UEFI isn't marking read-only *data* sections read-only yet for some reason; only R+X sections). But still that basically means that new versions of UEFI are going to stop booting all existing EFI Xen images, doesn't it? Perhaps we should look for a mitigation on the UEFI side. Jeff, Jiewen, has this actually been shipped in an EDK2 release yet? I confess I haven't actually built a current OVMF and *tested* the hypothesis that it breaks; it just seems "obvious" :) Adding Mark. Background: we think https://github.com/tianocore/edk2/commit/d0e92aad46 will break existing Xen EFI binaries because they write to a read-only code section (.init.te) before calling ExitBootServices().
--- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -87,7 +87,8 @@ static void __init efi_arch_relocate_image(unsigned long delta) case PE_BASE_RELOC_DIR64: if ( in_page_tables(addr) ) blexit(L"Unexpected relocation type"); - *(u64 *)addr += delta; + if ( delta ) + *(u64 *)addr += delta; break; default: blexit(L"Unsupported relocation type");