| Message ID | 1501673413.20068.15.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> David Woodhouse <dwmw2@infradead.org> 08/02/17 1:30 PM >>> >--- 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"); While of course this and the other half of the necessary changes are independent (i.e. this patch could be taken as is), I really had intended to deal with both sides of this issue at once, and hence I'm not entirely happy for this partial change to go in on its own. Nevertheless if need be it can have my ack. Jan
On Wed, 2017-08-02 at 05:56 -0600, Jan Beulich wrote: > > > > > > > > > > > > > David Woodhouse <dwmw2@infradead.org> 08/02/17 1:30 PM >>> > > --- 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"); > > While of course this and the other half of the necessary changes are > independent (i.e. this patch could be taken as is), I really had intended > to deal with both sides of this issue at once, and hence I'm not entirely > happy for this partial change to go in on its own. Nevertheless if need > be it can have my ack. I am, evidently, still being dense. This change is sufficient (we believe) to make EFI builds of Xen actually boot again on current EDK2, is it not? What is the "other half" of which you speak? You mentioned marking the section RWX — but for that to be a long-term solution to the issue, we'd basically have to ensure that we always mark *all* sections which might have relocations (even .rodata) as writeable, in case UEFI decides to honour their read-only status at some point in the future.
>>> David Woodhouse <dwmw2@infradead.org> 08/02/17 2:11 PM >>> >On Wed, 2017-08-02 at 05:56 -0600, Jan Beulich wrote: >> > > > David Woodhouse <dwmw2@infradead.org> 08/02/17 1:30 PM >>> >> > --- 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"); >> >> While of course this and the other half of the necessary changes are >> independent (i.e. this patch could be taken as is), I really had intended >> to deal with both sides of this issue at once, and hence I'm not entirely >> happy for this partial change to go in on its own. Nevertheless if need >> be it can have my ack. > >I am, evidently, still being dense. > >This change is sufficient (we believe) to make EFI builds of Xen >actually boot again on current EDK2, is it not? It is safe / sufficient only with the specific behavior you've observed, i.e. when permission restrictions are being removed during ExitBootServices(). I don't recall having seen any statement to that effect in the spec, and even if there was such a statement surely we'd find a firmware vendor who doesn't play by it. >What is the "other half" of which you speak? You mentioned marking the >section RWX — but for that to be a long-term solution to the issue, >we'd basically have to ensure that we always mark *all* sections which >might have relocations (even .rodata) as writeable, in case UEFI >decides to honour their read-only status at some point in the future. The other half is to preferably remove all (assembly) contributions from sections ending up R or RX. In particular, just like the compiler does, such .rodata pieces ought to be moved to .data.rel.ro; .init.text ones would need to move to .init.data or .init.data.rel.ro. And ideally we'd have link time verification that no relocations exist for R or RX sections ... Jan
On Wed, 2017-08-02 at 07:58 -0600, Jan Beulich wrote: > > > > David Woodhouse <dwmw2@infradead.org> 08/02/17 2:11 PM >>> > > This change is sufficient (we believe) to make EFI builds of Xen > > actually boot again on current EDK2, is it not? > > It is safe / sufficient only with the specific behavior you've observed, i.e. > when permission restrictions are being removed during ExitBootServices(). > I don't recall having seen any statement to that effect in the spec, and > even if there was such a statement surely we'd find a firmware vendor > who doesn't play by it. I'd be relatively surprised if a vendor were to make changes to the underlying TianoCore/EDK2 implementation in this respect. It doesn't seem like one of the areas in which they would want to apply the "value subtract" that they so desperately cling to. Perhaps we should push to have the spec amended to mandate the current behaviour? > > > > What is the "other half" of which you speak? You mentioned marking the > > section RWX — but for that to be a long-term solution to the issue, > > we'd basically have to ensure that we always mark *all* sections which > > might have relocations (even .rodata) as writeable, in case UEFI > > decides to honour their read-only status at some point in the future. > > The other half is to preferably remove all (assembly) contributions from > sections ending up R or RX. In particular, just like the compiler does, > such .rodata pieces ought to be moved to .data.rel.ro; .init.text ones > would need to move to .init.data or .init.data.rel.ro. And ideally we'd have > link time verification that no relocations exist for R or RX sections ... It wouldn't be that hard to add build-time verification in mkreloc.c — it's already processing one section at a time, and can see the flags. So it shouldn't be hard to make it bail out if a section without the W flag contains any relocations. But we might do better just to mark all the COFF sections as writeable, even if it's done by post-processing the EFI executable. The *important* part is marking them read-only in our own page tables after we're running, and grouping them into sections to make that possible is most useful. UEFI marking them read-only in the brief period while we're starting up is just kind of pointless from our point of view. Alternatively, if we really don't trust the UEFI implementation to let us write to read-only sections after ExitBootServices, we could ensure that we switch to our own page tables *before* doing the final pass of relocations. Currently efi_arch_post_exit_boot() will do them just *before* the switch. That's slightly less trivial than it sounds, as it would need to be position-independent. But doesn't it basically already need to be, or it would end up relocating itself while it's running? (Hm, ick... the more I look at this, the more I wish my initial response had been "la la la it was already broken it wasn't me..." :)
>>> David Woodhouse <dwmw2@infradead.org> 08/02/17 4:45 PM >>> >On Wed, 2017-08-02 at 07:58 -0600, Jan Beulich wrote: >> > > > David Woodhouse <dwmw2@infradead.org> 08/02/17 2:11 PM >>> >> > This change is sufficient (we believe) to make EFI builds of Xen >> > actually boot again on current EDK2, is it not? >> >> It is safe / sufficient only with the specific behavior you've observed, i.e. >> when permission restrictions are being removed during ExitBootServices(). >> I don't recall having seen any statement to that effect in the spec, and >> even if there was such a statement surely we'd find a firmware vendor >> who doesn't play by it. > >I'd be relatively surprised if a vendor were to make changes to the >underlying TianoCore/EDK2 implementation in this respect. It doesn't >seem like one of the areas in which they would want to apply the "value >subtract" that they so desperately cling to. Well, I've seen breakage in all sorts of places I wouldn't have expected anyone to fine a need to fiddle with. >Perhaps we should push to have the spec amended to mandate the current >behaviour? That would be nice, but wouldn't keep people from still getting it wrong, I'm afraid. >(Hm, ick... the more I look at this, the more I wish my initial >response had been "la la la it was already broken it wasn't me..." :) ;-) Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index bedac5c..8d295ff 100644 --- 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");
The function is invoked with delta=0 before ExitBootServices() is called, as a dummy run purely to validate that all the relocations can be handled. This allows us to exit gracefully with an error message. However, we have relocations in read-only sections such as .rodata and .init.te(xt). Recent versions of UEFI will actually make those sections read-only, which will cause a fault. This functionaity was added in EDK2 commit d0e92aad4 ("MdeModulePkg/DxeCore: Add UEFI image protection.") It's OK to actually make the changes in the later pass because UEFI will tear down the protection when ExitBootServices() is called, because it knows we're going to need to do this kind of thing. Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> --- This basically means that new versions of UEFI are going to break (all?) existing EFI Xen versions? Or at least any that have relocations in .init.text. xen/arch/x86/efi/efi-boot.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)