| Message ID | 1458218596-23847-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.03.16 at 13:43, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -529,9 +529,33 @@ static void noinline init_done(void) > } > else > { > + /* Mark .text as RX (avoiding the first 2M superpage). */ > + map_pages_to_xen(XEN_VIRT_START + MB(2), > + PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), > + PFN_DOWN(__2M_text_end - > + (const char *)(XEN_VIRT_START + MB(2))), > + PAGE_HYPERVISOR_RX); > + > + /* Mark .rodata as RO. */ > + map_pages_to_xen((unsigned long)&__2M_rodata_start, > + PFN_DOWN(__pa(__2M_rodata_start)), > + PFN_DOWN(__2M_rodata_end - __2M_rodata_start), > + PAGE_HYPERVISOR_RO); > + > + /* Free and reuse .init. */ > destroy_xen_mappings((unsigned long)&__init_begin, > (unsigned long)&__init_end); > init_xenheap_pages(__pa(__init_begin), __pa(__init_end)); > + > + /* Mark .data and .bss as RW. */ > + map_pages_to_xen((unsigned long)&__2M_rwdata_start, > + PFN_DOWN(__pa(__2M_rwdata_start)), > + PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), > + PAGE_HYPERVISOR_RW); > + > + /* Drop the remaining mappings in the shattered superpage. */ > + destroy_xen_mappings((unsigned long)&__2M_rwdata_end, > + ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2))); > } I think this would be more appropriate to add to some __init function (which the discarding of .init.* naturally can't live in). Also - do we really want to make this code dependent on map_pages_to_xen() not intermediately zapping the mappings being changed? Jan
On 17/03/16 14:31, Jan Beulich wrote: >>>> On 17.03.16 at 13:43, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -529,9 +529,33 @@ static void noinline init_done(void) >> } >> else >> { >> + /* Mark .text as RX (avoiding the first 2M superpage). */ >> + map_pages_to_xen(XEN_VIRT_START + MB(2), >> + PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), >> + PFN_DOWN(__2M_text_end - >> + (const char *)(XEN_VIRT_START + MB(2))), >> + PAGE_HYPERVISOR_RX); >> + >> + /* Mark .rodata as RO. */ >> + map_pages_to_xen((unsigned long)&__2M_rodata_start, >> + PFN_DOWN(__pa(__2M_rodata_start)), >> + PFN_DOWN(__2M_rodata_end - __2M_rodata_start), >> + PAGE_HYPERVISOR_RO); >> + >> + /* Free and reuse .init. */ >> destroy_xen_mappings((unsigned long)&__init_begin, >> (unsigned long)&__init_end); >> init_xenheap_pages(__pa(__init_begin), __pa(__init_end)); >> + >> + /* Mark .data and .bss as RW. */ >> + map_pages_to_xen((unsigned long)&__2M_rwdata_start, >> + PFN_DOWN(__pa(__2M_rwdata_start)), >> + PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), >> + PAGE_HYPERVISOR_RW); >> + >> + /* Drop the remaining mappings in the shattered superpage. */ >> + destroy_xen_mappings((unsigned long)&__2M_rwdata_end, >> + ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2))); >> } > I think this would be more appropriate to add to some __init > function (which the discarding of .init.* naturally can't live in). I will see if I can pull it forwards to just after the relocation, to be as close to the permissions change in the 2M case as possible. > > Also - do we really want to make this code dependent on > map_pages_to_xen() not intermediately zapping the mappings > being changed? Do you mean "immediately"? As far as I can tell, it is guaranteed to be safe, even when remapping the code section. Updates to the live pagetables are using atomic writes, and I didn't spot a point which would end up with a transient non-present mapping. ~Andrew
>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote: > On 17/03/16 14:31, Jan Beulich wrote: >> Also - do we really want to make this code dependent on >> map_pages_to_xen() not intermediately zapping the mappings >> being changed? > > Do you mean "immediately"? No. > As far as I can tell, it is guaranteed to be safe, even when remapping > the code section. Updates to the live pagetables are using atomic > writes, and I didn't spot a point which would end up with a transient > non-present mapping. But we may, at some point and for whatever reason, come to make the function zap the mapping (i.e. clear the present bit), flush, and only the re-establish the new mapping. Jan
On 17/03/16 15:32, Jan Beulich wrote: >>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote: >> On 17/03/16 14:31, Jan Beulich wrote: >>> Also - do we really want to make this code dependent on >>> map_pages_to_xen() not intermediately zapping the mappings >>> being changed? >> Do you mean "immediately"? > No. > >> As far as I can tell, it is guaranteed to be safe, even when remapping >> the code section. Updates to the live pagetables are using atomic >> writes, and I didn't spot a point which would end up with a transient >> non-present mapping. > But we may, at some point and for whatever reason, come to make > the function zap the mapping (i.e. clear the present bit), flush, and > only the re-establish the new mapping. This change is temporary until I can fix the legacy boot issue and reintroduce the proper 2M functionality. If someone in the future wants to change the behaviour of map_pages_to_xen() then we can reconsider. However, I think it is unlikely that this will actually happen at all, and if it ever does, I hope to have already fixed the 2M alignment and deleted this change. This change is a big security improvement, and absolutely should be taken, especially as the current implementation of map_pages_to_xen() is safe. ~Andrew
>>> On 17.03.16 at 17:15, <andrew.cooper3@citrix.com> wrote: > On 17/03/16 15:32, Jan Beulich wrote: >>>>> On 17.03.16 at 15:44, <andrew.cooper3@citrix.com> wrote: >>> On 17/03/16 14:31, Jan Beulich wrote: >>>> Also - do we really want to make this code dependent on >>>> map_pages_to_xen() not intermediately zapping the mappings >>>> being changed? >>> Do you mean "immediately"? >> No. >> >>> As far as I can tell, it is guaranteed to be safe, even when remapping >>> the code section. Updates to the live pagetables are using atomic >>> writes, and I didn't spot a point which would end up with a transient >>> non-present mapping. >> But we may, at some point and for whatever reason, come to make >> the function zap the mapping (i.e. clear the present bit), flush, and >> only the re-establish the new mapping. > > This change is temporary until I can fix the legacy boot issue and > reintroduce the proper 2M functionality. > > If someone in the future wants to change the behaviour of > map_pages_to_xen() then we can reconsider. However, I think it is > unlikely that this will actually happen at all, and if it ever does, I > hope to have already fixed the 2M alignment and deleted this change. > > This change is a big security improvement, and absolutely should be > taken, especially as the current implementation of map_pages_to_xen() is > safe. I by no means intend to reject this change just because of this aspect - I merely wanted to make the slight concern explicit. Jan
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c5c332d..e278a7b 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -529,9 +529,33 @@ static void noinline init_done(void) } else { + /* Mark .text as RX (avoiding the first 2M superpage). */ + map_pages_to_xen(XEN_VIRT_START + MB(2), + PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), + PFN_DOWN(__2M_text_end - + (const char *)(XEN_VIRT_START + MB(2))), + PAGE_HYPERVISOR_RX); + + /* Mark .rodata as RO. */ + map_pages_to_xen((unsigned long)&__2M_rodata_start, + PFN_DOWN(__pa(__2M_rodata_start)), + PFN_DOWN(__2M_rodata_end - __2M_rodata_start), + PAGE_HYPERVISOR_RO); + + /* Free and reuse .init. */ destroy_xen_mappings((unsigned long)&__init_begin, (unsigned long)&__init_end); init_xenheap_pages(__pa(__init_begin), __pa(__init_end)); + + /* Mark .data and .bss as RW. */ + map_pages_to_xen((unsigned long)&__2M_rwdata_start, + PFN_DOWN(__pa(__2M_rwdata_start)), + PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), + PAGE_HYPERVISOR_RW); + + /* Drop the remaining mappings in the shattered superpage. */ + destroy_xen_mappings((unsigned long)&__2M_rwdata_end, + ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2))); } printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10); diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 961f48f..ab8ba74 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -55,6 +55,8 @@ SECTIONS #ifdef EFI . = ALIGN(MB(2)); +#else + . = ALIGN(PAGE_SIZE); #endif __2M_text_end = .; @@ -98,6 +100,8 @@ SECTIONS #ifdef EFI . = ALIGN(MB(2)); +#else + . = ALIGN(PAGE_SIZE); #endif __2M_rodata_end = .; @@ -167,6 +171,8 @@ SECTIONS #ifdef EFI . = ALIGN(MB(2)); +#else + . = ALIGN(PAGE_SIZE); #endif __2M_init_end = .; @@ -211,6 +217,8 @@ SECTIONS #ifdef EFI . = ALIGN(MB(2)); +#else + . = ALIGN(PAGE_SIZE); #endif __2M_rwdata_end = .; @@ -269,6 +277,14 @@ ASSERT(IS_ALIGNED(__2M_init_start, MB(2)), "__2M_init_start misaligned") ASSERT(IS_ALIGNED(__2M_init_end, MB(2)), "__2M_init_end misaligned") ASSERT(IS_ALIGNED(__2M_rwdata_start, MB(2)), "__2M_rwdata_start misaligned") ASSERT(IS_ALIGNED(__2M_rwdata_end, MB(2)), "__2M_rwdata_end misaligned") +#else +ASSERT(IS_ALIGNED(__2M_text_end, PAGE_SIZE), "__2M_text_end misaligned") +ASSERT(IS_ALIGNED(__2M_rodata_start, PAGE_SIZE), "__2M_rodata_start misaligned") +ASSERT(IS_ALIGNED(__2M_rodata_end, PAGE_SIZE), "__2M_rodata_end misaligned") +ASSERT(IS_ALIGNED(__2M_init_start, PAGE_SIZE), "__2M_init_start misaligned") +ASSERT(IS_ALIGNED(__2M_init_end, PAGE_SIZE), "__2M_init_end misaligned") +ASSERT(IS_ALIGNED(__2M_rwdata_start, PAGE_SIZE), "__2M_rwdata_start misaligned") +ASSERT(IS_ALIGNED(__2M_rwdata_end, PAGE_SIZE), "__2M_rwdata_end misaligned") #endif ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
c/s cf39362 "x86: use 2M superpages for text/data/bss mappings" served two purposes; to map the primary code and data with appropriate pagetable permissions (rather than unilaterally RWX), and to reduce the TLB pressure. The extra alignment exposed a SYSLinux issue, and was partly reverted by c/s 0b8a172 "x86: partially revert use of 2M mappings for hypervisor image". This change reinstates the pagetable permission improvements while avoiding the 2M alignment issue. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/setup.c | 24 ++++++++++++++++++++++++ xen/arch/x86/xen.lds.S | 16 ++++++++++++++++ 2 files changed, 40 insertions(+)