Re: [Xen-devel] [PATCH] x86/efi: Do not write relocations in efi_arch_relocate_image() first pass

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> David Woodhouse <dwmw2@xxxxxxxxxxxxx> 08/02/17 2:11 PM >>>
>On Wed, 2017-08-02 at 05:56 -0600, Jan Beulich wrote:
>> > > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel