Re: [Xen-devel] [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
>>> On 17.03.16 at 13:43,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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
On 17/03/16 15:32, Jan Beulich wrote: On 17.03.16 at 15:44,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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
>>> On 17.03.16 at 15:44,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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
On 17/03/16 14:31, Jan Beulich wrote: On 17.03.16 at 13:43,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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: Remap text/data/bss with appropriate permissions
>>> On 17.03.16 at 17:15,wrote: > On 17/03/16 15:32, Jan Beulich wrote: > On 17.03.16 at 15:44, 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel