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

Commit Message

David Woodhouse July 20, 2017, 3:20 p.m. UTC

From: David Woodhouse <[e-mail]>

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 <[e-mail]>
---
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(-)

-- 
2.7.4

Comments

Wei Liu July 20, 2017, 4:46 p.m. UTC | #1

CC relevant maintainers

On Thu, Jul 20, 2017 at 05:20:43PM +0200, David Woodhouse wrote:
> From: David Woodhouse <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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
> [e-mail]
> https://lists.xen.org/xen-devel

Wei Liu July 20, 2017, 4:54 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>

Reviewed-by: Wei Liu <[e-mail]>

Julien Grall July 21, 2017, 10:43 a.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>
> Reviewed-by: Wei Liu <[e-mail]>

Acked-by: Julien Grall <[e-mail]>

Cheers,

Andrew Cooper July 21, 2017, 4:41 p.m. UTC | #4

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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>
>> Reviewed-by: Wei Liu <[e-mail]>
>
> Acked-by: Julien Grall <[e-mail]>

Acked-by: Andrew Cooper <[e-mail]>

Jan Beulich July 30, 2017, 6:16 a.m. UTC | #5

>>> David Woodhouse <[e-mail]> 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

Andrew Cooper July 30, 2017, 12:50 p.m. UTC | #6

On 30/07/17 07:16, Jan Beulich wrote:
>>>> David Woodhouse <[e-mail]> 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

David Woodhouse July 30, 2017, 11:18 p.m. UTC | #7

On Sun, 2017-07-30 at 13:50 +0100, Andrew Cooper wrote:
> On 30/07/17 07:16, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > > 
> > > > > David Woodhouse <[e-mail]> 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?

Jan Beulich July 31, 2017, 9:55 a.m. UTC | #8

>>> Andrew Cooper <[e-mail]> 07/30/17 2:50 PM >>>
>On 30/07/17 07:16, Jan Beulich wrote:
>>>>> David Woodhouse <[e-mail]> 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

Jan Beulich July 31, 2017, 9:57 a.m. UTC | #9

>>> David Woodhouse <[e-mail]> 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 <[e-mail]> 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

David Woodhouse July 31, 2017, 11:02 a.m. UTC | #10

On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > David Woodhouse <[e-mail]> 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.

Jan Beulich July 31, 2017, 1:15 p.m. UTC | #11

>>> David Woodhouse <[e-mail]> 07/31/17 1:02 PM >>>
>On Sun, 2017-07-30 at 00:16 -0600, Jan Beulich wrote:
>> > > > David Woodhouse <[e-mail]> 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

Patch

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