Message ID | 1500564043.4400.15.camel@infradead.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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>
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,
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>
>>> 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
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
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?
>>> 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
>>> 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
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.
>>> 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
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 */