Re: [Xen-devel] xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Mon, 2017-07-31 at 07:15 -0600, Jan Beulich wrote:
> > > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> 07/31/17 1:02 PM >>>
> > On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
> > > > > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> 07/20/17 5:22 PM >>>
> > > > This includes stuff lke the hypercall tables which we really want
> > > > to be read-only. And they were going into .data.read-mostly.
> > >
> > > Yes, we'd like them to be read-only, but what if EFI properly assigned r/o
> > > permissions to the .rodata section when loading xen.efi? We'd then be
> > > unable to apply relocations when switching from 1:1 to virtual mappings
> > > (see efi_arch_relocate_image()).
> > 
> > FWIW it does look like TianoCore has gained the ability to mark
> > sections as read-only, in January of this year:
> > https://github.com/tianocore/edk2/commit/d0e92aad46
> > 
> > It doesn't actually seem to be complete — even with subsequent fixes
> > since that commit, it doesn't look like it catches the case of data
> > sections without EFI_IMAGE_SCN_MEM_WRITE, such as .rodata. 
> > 
> > And even if/when that gets fixed you'll note that the protection is
> > deliberately torn down in ExitBootServices(), specifically for the case
> > you're concerned about below — because you'll need to do the
> > relocations.
>
> As said in an earlier reply, a first pass over relocations is being done
> long before the call to ExitBootServices(). A minimal adjustment to
> efi_arch_relocate_image() will be needed anyway, afaict.

Ah, right. I think more "implied" than "said" but I understand now. :)

At least, I understand what you're saying... I have no bloody clue
what's actually going on though.

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;
                 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,
but what? Is it still supposed to be adding xen_phys_start in the
PE_BASE_RELOC_HIGHLOW case even when delta==0? Because it isn't...

Either way, this is still broken before my patch though, right?
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.

And the .init.te(xt) relocations include PE_BASE_RELOC_DIR64
relocations, which *would* cause a fault in the !delta case.

(All the relocations in .rodata both before and after my patch are also
PE_BASE_RELOC_DIR64, FWIW)

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

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