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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Wed, 2017-08-02 at 07:58 -0600, Jan Beulich wrote:
> > > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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..." :)

Attachment: smime.p7s
Description: S/MIME cryptographic signature

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