[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/8] x86/mm: Drop bogus cacheability logic in update_xen_mappings()
On 30/11/2021 10:04, Andrew Cooper wrote: > There is no circumstance under which any part of the Xen image in memory wants > to have any cacheability other than Write Back. > > Furthermore, unmapping random pieces of Xen like that comes with a non-trivial > risk of a Triple Fault, or other fatal condition. Even if it appears to work, > an NMI or interrupt as a far wider reach into Xen mappings than calling > map_pages_to_xen() thrice. > > Simplfy the safety checking to a BUG_ON(). It is substantially more correct > and robust than following either of the unlikely(alias) paths. > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Wei Liu <wl@xxxxxxx> > > I'm half tempted to drop the check entirely, but in that case it would be > better to inline the result into the two callers. > --- > xen/arch/x86/mm.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 4d799032dc82..9bd4e5cc1d2f 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -785,24 +785,21 @@ bool is_iomem_page(mfn_t mfn) > > static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr) > { > - int err = 0; > bool alias = mfn >= PFN_DOWN(xen_phys_start) && > mfn < PFN_UP(xen_phys_start + xen_virt_end - XEN_VIRT_START); > - unsigned long xen_va = > - XEN_VIRT_START + ((mfn - PFN_DOWN(xen_phys_start)) << PAGE_SHIFT); > + > + /* > + * Something is catastrophically broken if someone is trying to change > the > + * cacheability of Xen in memory... > + */ > + BUG_ON(alias); > > if ( boot_cpu_has(X86_FEATURE_XEN_SELFSNOOP) ) > return 0; > > - if ( unlikely(alias) && cacheattr ) > - err = map_pages_to_xen(xen_va, _mfn(mfn), 1, 0); > - if ( !err ) > - err = map_pages_to_xen((unsigned long)mfn_to_virt(mfn), _mfn(mfn), 1, > - PAGE_HYPERVISOR | cacheattr_to_pte_flags(cacheattr)); > - if ( unlikely(alias) && !cacheattr && !err ) > - err = map_pages_to_xen(xen_va, _mfn(mfn), 1, PAGE_HYPERVISOR); > - > - return err; > + return map_pages_to_xen( > + (unsigned long)mfn_to_virt(mfn), _mfn(mfn), 1, > + PAGE_HYPERVISOR | cacheattr_to_pte_flags(cacheattr)); In light of the discussion on patch 1, this is no longer safe. The alias calculation includes 2M of arbitrary guest memory, and in principle there are legitimate reasons for a guest to want to map RAM as WC (e.g. GPU pagetables) or in the future, WP (in place RAM encryption/decryption). The gross hack fix would be "mfn >= PFN_DOWN(xen_phys_start + MB(2))", but but this is screaming for a helper. xen_in_range() is part-way there, but is an O(n) loop over the regions. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |