xen/link: Move .data.rel.ro sections into .rodata for final link

xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1776 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

This includes stuff lke the hypercall tables which we really want
to be read-only. And they were going into .data.read-mostly.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
Build tested on x86_64 (you really don't want to know about what I
*actually* tested it with), not at all tested on ARM.

 xen/arch/arm/xen.lds.S | 4 ++--
 xen/arch/x86/xen.lds.S | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 44bd3bf..2d54f22 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -52,6 +52,8 @@ SECTIONS
        __stop_bug_frames_2 = .;
        *(.rodata)
        *(.rodata.*)
+       *(.data.rel.ro)
+       *(.data.rel.ro.*)
 
 #ifdef CONFIG_LOCK_PROFILE
        . = ALIGN(POINTER_ALIGN);
@@ -97,8 +99,6 @@ SECTIONS
        __stop___pre_ex_table = .;
 
        *(.data.read_mostly)
-       *(.data.rel.ro)
-       *(.data.rel.ro.*)
   } :text
 
   . = ALIGN(8);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8289a1b..ff08bbe 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -90,6 +90,8 @@ SECTIONS
 
        *(.rodata)
        *(.rodata.*)
+       *(.data.rel.ro)
+       *(.data.rel.ro.*)
 
 #if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI)
 /*
@@ -224,8 +226,6 @@ SECTIONS
        __start_schedulers_array = .;
        *(.data.schedulers)
        __end_schedulers_array = .;
-       *(.data.rel.ro)
-       *(.data.rel.ro.*)
   } :text
 
   .data : {                    /* Data */
-- 
2.7.4

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Wei Liu]

CC relevant maintainers

On Thu, Jul 20, 2017 at 05:20:43PM +0200, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This includes stuff lke the hypercall tables which we really want
> to be read-only. And they were going into .data.read-mostly.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> Build tested on x86_64 (you really don't want to know about what I
> *actually* tested it with), not at all tested on ARM.
> 
>  xen/arch/arm/xen.lds.S | 4 ++--
>  xen/arch/x86/xen.lds.S | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 44bd3bf..2d54f22 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -52,6 +52,8 @@ SECTIONS
>         __stop_bug_frames_2 = .;
>         *(.rodata)
>         *(.rodata.*)
> +       *(.data.rel.ro)
> +       *(.data.rel.ro.*)
>  
>  #ifdef CONFIG_LOCK_PROFILE
>         . = ALIGN(POINTER_ALIGN);
> @@ -97,8 +99,6 @@ SECTIONS
>         __stop___pre_ex_table = .;
>  
>         *(.data.read_mostly)
> -       *(.data.rel.ro)
> -       *(.data.rel.ro.*)
>    } :text
>  
>    . = ALIGN(8);
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 8289a1b..ff08bbe 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -90,6 +90,8 @@ SECTIONS
>  
>         *(.rodata)
>         *(.rodata.*)
> +       *(.data.rel.ro)
> +       *(.data.rel.ro.*)
>  
>  #if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI)
>  /*
> @@ -224,8 +226,6 @@ SECTIONS
>         __start_schedulers_array = .;
>         *(.data.schedulers)
>         __end_schedulers_array = .;
> -       *(.data.rel.ro)
> -       *(.data.rel.ro.*)
>    } :text
>  
>    .data : {                    /* Data */
> -- 
> 2.7.4



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


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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Wei Liu]

On Thu, Jul 20, 2017 at 05:46:50PM +0100, Wei Liu wrote:
> CC relevant maintainers
> 
> On Thu, Jul 20, 2017 at 05:20:43PM +0200, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > This includes stuff lke the hypercall tables which we really want

lke -> like

> > to be read-only. And they were going into .data.read-mostly.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Julien Grall]

On 20/07/17 17:54, Wei Liu wrote:
> On Thu, Jul 20, 2017 at 05:46:50PM +0100, Wei Liu wrote:
>> CC relevant maintainers
>>
>> On Thu, Jul 20, 2017 at 05:20:43PM +0200, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> This includes stuff lke the hypercall tables which we really want
>
> lke -> like
>
>>> to be read-only. And they were going into .data.read-mostly.
>>>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Andrew Cooper]

On 21/07/17 11:43, Julien Grall wrote:
>
>
> On 20/07/17 17:54, Wei Liu wrote:
>> On Thu, Jul 20, 2017 at 05:46:50PM +0100, Wei Liu wrote:
>>> CC relevant maintainers
>>>
>>> On Thu, Jul 20, 2017 at 05:20:43PM +0200, David Woodhouse wrote:
>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>
>>>> This includes stuff lke the hypercall tables which we really want
>>
>> lke -> like
>>
>>>> to be read-only. And they were going into .data.read-mostly.
>>>>
>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>
>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: Julien Grall <julien.grall@arm.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH v2] xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1856 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

This includes stuff like the hypercall tables which we really kind of want
to be read-only. And they were going into .data.read-mostly.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Fixed typo, collected acks, and this time sent from a properly
maintained distribution instead of Ubuntu, so spaces shouldn't get
turned into &nbsp; by the mailer.

 xen/arch/arm/xen.lds.S | 4 ++--
 xen/arch/x86/xen.lds.S | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 44bd3bf..2d54f22 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -52,6 +52,8 @@ SECTIONS
        __stop_bug_frames_2 = .;
        *(.rodata)
        *(.rodata.*)
+       *(.data.rel.ro)
+       *(.data.rel.ro.*)
 
 #ifdef CONFIG_LOCK_PROFILE
        . = ALIGN(POINTER_ALIGN);
@@ -97,8 +99,6 @@ SECTIONS
        __stop___pre_ex_table = .;
 
        *(.data.read_mostly)
-       *(.data.rel.ro)
-       *(.data.rel.ro.*)
   } :text
 
   . = ALIGN(8);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8289a1b..ff08bbe 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -90,6 +90,8 @@ SECTIONS
 
        *(.rodata)
        *(.rodata.*)
+       *(.data.rel.ro)
+       *(.data.rel.ro.*)
 
 #if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI)
 /*
@@ -224,8 +226,6 @@ SECTIONS
        __start_schedulers_array = .;
        *(.data.schedulers)
        __end_schedulers_array = .;
-       *(.data.rel.ro)
-       *(.data.rel.ro.*)
   } :text
 
   .data : {                    /* Data */
-- 
2.7.4

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Jan Beulich]

>>> David Woodhouse <dwmw2@infradead.org> 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()).

Jan


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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Andrew Cooper]

On 30/07/17 07:16, Jan Beulich wrote:
>>>> David Woodhouse <dwmw2@infradead.org> 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()).

Ah yes.  I'd overlooked that point when considering the ramifications of
this change.

efi_arch_relocate_image() should probably do the same as what we do with
livepatching, and temporarily clear CR0.WP for the duration of the patching.

~Andrew

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1005 bytes --]

On Sun, 2017-07-30 at 13:50 +0100, Andrew Cooper wrote:
> On 30/07/17 07:16, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > > 
> > > > > David Woodhouse <dwmw2@infradead.org> 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()).
> Ah yes.  I'd overlooked that point when considering the ramifications of
> this change.
> 
> efi_arch_relocate_image() should probably do the same as what we do with
> livepatching, and temporarily clear CR0.WP for the duration of the patching.

Hm, efi/mkreloc.c was already emitting relocations in the .rodata
section before this change. Are you saying that was already broken?

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 07/30/17 2:50 PM >>>
>On 30/07/17 07:16, Jan Beulich wrote:
>>>>> David Woodhouse <dwmw2@infradead.org> 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()).
>
>Ah yes.  I'd overlooked that point when considering the ramifications of
>this change.
>
>efi_arch_relocate_image() should probably do the same as what we do with
>livepatching, and temporarily clear CR0.WP for the duration of the patching.

Yes, we could do that, but with some care - we should no play with CR0.WP
prior to ExitBootServices(), so we would need to avoid actually writing out
relocated values for that first pass even in the 64-bit reloc case.

Jan



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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Jan Beulich]

>>> David Woodhouse <dwmw2@infradead.org> 07/31/17 1:19 AM >>>
>On Sun, 2017-07-30 at 13:50 +0100, Andrew Cooper wrote:
>> On 30/07/17 07:16, Jan Beulich wrote:
>> > > > > David Woodhouse <dwmw2@infradead.org> 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()).
>> Ah yes.  I'd overlooked that point when considering the ramifications of
>> this change.
>> 
>> efi_arch_relocate_image() should probably do the same as what we do with
>> livepatching, and temporarily clear CR0.WP for the duration of the patching.
>
>Hm, efi/mkreloc.c was already emitting relocations in the .rodata
>section before this change. Are you saying that was already broken?

Are there any such relocations? The compiler shouldn't emit data needing
relocation to .rodata, so if at all such might live in assembly code. But yes,
if there are any, things would have been latently broken even before.

Jan


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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 2708 bytes --]

On Mon, 2017-07-31 at 03:57 -0600, Jan Beulich wrote:
> Are there any such relocations? The compiler shouldn't emit data needing
> relocation to .rodata, so if at all such might live in assembly code. But yes,
> if there are any, things would have been latently broken even before.


 $ git diff 33a0b4fe90f1ef1a104dd454c931bb46d417ffca^
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 93ead6e..aa25dd9 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -194,7 +194,7 @@ $(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/
        if $(guard) false; then rm -f $@; echo 'EFI support disabled'; \
        else $(NM) -pa --format=sysv $(@D)/$(@F) \
                | $(BASEDIR)/tools/symbols --xensyms --sysv --sort >$(@D)/$(@F).map; fi
-       rm -f $(@D)/.$(@F).[0-9]*
+       #rm -f $(@D)/.$(@F).[0-9]*
 
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index bddcce0..55d14a7 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -346,6 +346,7 @@ int main(int argc, char *argv[])
              memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 )
             continue;
 
+       printf("# section %.*s\n", (int)sizeof(sec1[i].name), sec1[i].name);
         if ( !sec1[i].rva )
         {
             fprintf(stderr, "Can't handle section %u with zero RVA\n", i);
 $ grep -A36 rodata .xen.efi.0r.S
# section .rodata
	.equ rva_00200000_relocs, 0x00000c
	.balign 4
	.long 0x41b000, rva_0041b000_relocs
	.word (10 << 12) | 0x920
	.word (10 << 12) | 0x928
	.word (10 << 12) | 0x930
	.word (10 << 12) | 0x938
	.word (10 << 12) | 0x940
	.word (10 << 12) | 0x948
	.word (10 << 12) | 0x950
	.word (10 << 12) | 0x958
	.word (10 << 12) | 0x960
	.word (10 << 12) | 0x968
	.word (10 << 12) | 0x970
	.word (10 << 12) | 0x978
	.word (10 << 12) | 0x980
	.word (10 << 12) | 0x988
	.word (10 << 12) | 0x990
	.word (10 << 12) | 0x998
	.word (10 << 12) | 0x9a0
	.word (10 << 12) | 0x9a8
	.word (10 << 12) | 0x9b0
	.word (10 << 12) | 0x9b8
	.word (10 << 12) | 0x9c0
	.word (10 << 12) | 0x9c8
	.word (10 << 12) | 0x9d0
	.word (10 << 12) | 0x9d8
	.word (10 << 12) | 0x9e0
	.word (10 << 12) | 0x9e8
	.word (10 << 12) | 0x9f0
	.word (10 << 12) | 0x9f8
	.word (10 << 12) | 0xa00
	.word (10 << 12) | 0xa08
	.word (10 << 12) | 0xa10
	.word (10 << 12) | 0xa18
# section .init.te

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1194 bytes --]

On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > David Woodhouse <dwmw2@infradead.org> 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.

So I don't think there should be a problem here.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Jan Beulich]

>>> David Woodhouse <dwmw2@infradead.org> 07/31/17 1:02 PM >>>
>On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
>> > > > David Woodhouse <dwmw2@infradead.org> 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.

Jan


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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 4446 bytes --]

On Mon, 2017-07-31 at 07:15 -0600, Jan Beulich wrote:
> > > > David Woodhouse <dwmw2@infradead.org> 07/31/17 1:02 PM >>>
> > On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
> > > > > > David Woodhouse <dwmw2@infradead.org> 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 #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[Jan Beulich]

>>> David Woodhouse <dwmw2@infradead.org> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1647 bytes --]

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(-)

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");
-- 
2.7.4

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: xen/link: Move .data.rel.ro sections into .rodata for final link

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1703 bytes --]

On Wed, 2017-08-02 at 03:17 -0600, Jan Beulich wrote:
> 
> >... 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.

Ah, thanks. I'd glossed over that as a "can never happen" condition.
And the 'default:' case really is that — the build system is broken if
we ever see that. But the DIR64 / in_page_tables() one is less provably
impossible.

> >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.

And more to the point, .init.text entries needing relocation (since
UEFI isn't marking read-only *data* sections read-only yet for some
reason; only R+X sections).

But still that basically means that new versions of UEFI are going to
stop booting all existing EFI Xen images, doesn't it? Perhaps we should
look for a mitigation on the UEFI side.

Jeff, Jiewen, has this actually been shipped in an EDK2 release yet?

I confess I haven't actually built a current OVMF and *tested* the
hypothesis that it breaks; it just seems "obvious" :)

Adding Mark. Background: we think
https://github.com/tianocore/edk2/commit/d0e92aad46 will break existing
Xen EFI binaries because they write to a read-only code section
(.init.te) before calling ExitBootServices().


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> 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


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

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

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1510 bytes --]

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.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> 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


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

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

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 3010 bytes --]

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..." :)

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> 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


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

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

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1466 bytes --]

On Wed, 2017-08-02 at 09:16 -0600, Jan Beulich wrote:
> 
> Well, I've seen breakage in all sorts of places I wouldn't have expected
> anyone to fine a need to fiddle with.

This is the nature of 'value subtract', I suppose. 

How about something like this? Somewhat complicated by the fact that
COFF section names get truncated, so maybe there's a nicer place to put
it (or maybe we explicitly include .init.da into the .init.data output
section, in the efi.lds linker script, or something?)....

--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -346,6 +346,15 @@ int main(int argc, char *argv[])
              memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 )
             continue;
 
+       /* For sections with relocations, force them to be writeable */
+       if (memcmp(sec1[i].name, ".init.da", sizeof(sec1[i].name)) == 0)
+               printf(".pushsection .init.data, \"awx\"\n");
+       else if (memcmp(sec1[i].name, ".init.te", sizeof(sec1[i].name) ) == 0)
+               printf(".pushsection .init.text, \"awx\"\n");
+       else
+               printf(".pushsection %.*s, \"awx\"\n", (int)sizeof(sec1[i].name), sec1[i].name);
+       printf(".popsection\n");
+
         if ( !sec1[i].rva )
         {
             fprintf(stderr, "Can't handle section %u with zero RVA\n", i);

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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