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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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


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