| Message ID | 20211130100445.31156-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for __ro_after_init | expand |
On 30.11.2021 11:04, Andrew Cooper wrote: > It has been 4 years since the default load address changed from 1M to 2M, and > _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused > mapping. > > To ensure we don't create mappings accidentally, loop from 0 and obey > _PAGE_PRESENT on all entries. > > Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> I guess this may be worth backporting despite not having any immediate adverse effect. Jan
On 30/11/2021 10:33, Jan Beulich wrote: > On 30.11.2021 11:04, Andrew Cooper wrote: >> It has been 4 years since the default load address changed from 1M to 2M, and >> _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused >> mapping. >> >> To ensure we don't create mappings accidentally, loop from 0 and obey >> _PAGE_PRESENT on all entries. >> >> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > I guess this may be worth backporting despite not having any immediate > adverse effect. I'd say so, yes. I too can't see an adverse effect right now, but I'm definitely wary of stray executable mappings lying around. In principle, it would be nice to reclaim the 2M of space (which only exists for the MB1 path IIRC), but then we're getting into a position where xen_phys_start isn't really that any more. A different alternative could be to use it for early memory allocations in Xen and treat it like .data, similar to Linux's early BRK(). This might simplify some of the relocation hoops we currently jump through. ~Andrew
On 30.11.2021 12:14, Andrew Cooper wrote: > On 30/11/2021 10:33, Jan Beulich wrote: >> On 30.11.2021 11:04, Andrew Cooper wrote: >>> It has been 4 years since the default load address changed from 1M to 2M, and >>> _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused >>> mapping. >>> >>> To ensure we don't create mappings accidentally, loop from 0 and obey >>> _PAGE_PRESENT on all entries. >>> >>> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> I guess this may be worth backporting despite not having any immediate >> adverse effect. > > I'd say so, yes. I too can't see an adverse effect right now, but I'm > definitely wary of stray executable mappings lying around. > > > In principle, it would be nice to reclaim the 2M of space (which only > exists for the MB1 path IIRC), but then we're getting into a position > where xen_phys_start isn't really that any more. Well, xen_phys_base might be slightly more accurate, but apart from that I do think that we reclaim that space (as much as we did reclaim the 1Mb before the change of the default load address): if ( efi_boot_mem_unused(&eb_start, &eb_end) ) { reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start)); reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end)); } else reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end)); Jan
On 30/11/2021 11:22, Jan Beulich wrote: > On 30.11.2021 12:14, Andrew Cooper wrote: >> On 30/11/2021 10:33, Jan Beulich wrote: >>> On 30.11.2021 11:04, Andrew Cooper wrote: >>>> It has been 4 years since the default load address changed from 1M to 2M, and >>>> _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused >>>> mapping. >>>> >>>> To ensure we don't create mappings accidentally, loop from 0 and obey >>>> _PAGE_PRESENT on all entries. >>>> >>>> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>> I guess this may be worth backporting despite not having any immediate >>> adverse effect. >> I'd say so, yes. I too can't see an adverse effect right now, but I'm >> definitely wary of stray executable mappings lying around. >> >> >> In principle, it would be nice to reclaim the 2M of space (which only >> exists for the MB1 path IIRC), but then we're getting into a position >> where xen_phys_start isn't really that any more. > Well, xen_phys_base might be slightly more accurate, but apart from that > I do think that we reclaim that space (as much as we did reclaim the 1Mb > before the change of the default load address): > > if ( efi_boot_mem_unused(&eb_start, &eb_end) ) > { > reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start)); > reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end)); > } > else > reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end)); That means there are zero safety barriers between a bad function pointer and executing arbitrary guest memory, doesn't it... My "adverse effect" comment was under the impression that we just left the range unused. ~Andrew
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index da47cdea14a1..6f241048425c 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* The only data mappings to be relocated are in the Xen area. */ pl2e = __va(__pa(l2_xenmap)); - /* - * Undo the temporary-hooking of the l1_directmap. __2M_text_start - * is contained in this PTE. - */ + BUG_ON(using_2M_mapping() && l2_table_offset((unsigned long)_erodata) == l2_table_offset((unsigned long)_stext)); - *pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT, - PAGE_HYPERVISOR_RX | _PAGE_PSE); - for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) + + for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ ) { unsigned int flags;
It has been 4 years since the default load address changed from 1M to 2M, and _stext ceased residing in l2_xenmap[0]. We should not be inserting an unused mapping. To ensure we don't create mappings accidentally, loop from 0 and obey _PAGE_PRESENT on all entries. Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> Previously posted on its own. --- xen/arch/x86/setup.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)