[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC v2 1/4] x86/mm: Shadow and p2m changes for PV mem_access
>>> On 25.07.14 at 01:34, <aravindp@xxxxxxxxx> wrote: >>> + >>> + /* If the access is against the permissions, then send to >>> mem_event >>*/ >>> + switch ( p2ma ) >>> + { >>> + case p2m_access_r: >>> + violation = access_w || access_x; >>> + break; >>> + case p2m_access_rx: >>> + case p2m_access_rx2rw: >>> + violation = access_w; >>> + break; >>> + case p2m_access_rw: >>> + violation = access_x; >>> + break; >>> + case p2m_access_rwx: >>> + default: >>> + break; >>> + } >>> + >>> + /* >>> + * Do not police writes to guest memory from the Xen >>> hypervisor. >>> + * This keeps PV mem_access on par with HVM. Turn off CR0.WP >>here to >>> + * allow the write to go through if the guest has marked the >>> page as >>> + * writable. Turn it back on in the guest access functions >>> + * __copy_to_user / __put_user_size() after the write is >>> completed. >>> + */ >>> + if ( violation && access_w && >>> + regs->eip >= XEN_VIRT_START && regs->eip <= XEN_VIRT_END ) >> >>Definitely < instead of <= on the right side. But - is this safe, the more >>that this again doesn't appear to be sitting in a guest kind specific block? >>I'd at least expect this to be qualified by a regs->cs and/or >>guest_mode() check. > > I will add the guest kind check. Should I do it for the whole block i.e add > is_pv_domain() in addition to mem_event_check_ring()? Since mem_event_check_ring() isn't PV specific, you'd need to go through and amend any such checks with a PV one where needed. And yes, doing it once for the whole code block seems like the right thing to do here. > That will also address > next comment below. I will add the guest_mode() in addition to the above > check for policing Xen writes to guest memory. > >>> + { >>> + unsigned long cr0 = read_cr0(); >>> + >>> + violation = 0; >>> + if ( cr0 & X86_CR0_WP && >>> + guest_l1e_get_flags(gw.l1e) & _PAGE_RW ) >>> + { >>> + cr0 &= ~X86_CR0_WP; >>> + write_cr0(cr0); >>> + v->arch.pv_vcpu.need_cr0_wp_set = 1; >> >>PV field access within a non-PV-only code block? >> >>> + } >> >>I wonder how well the window where you're running with CR0.WP clear >>is bounded: The flag serves as a kind of security measure, and hence >>shouldn't be left off for extended periods of time. > > I agree. Is there a way I can bound this? That's what you need to figure out. The simplistic solution (single stepping just the critical instruction(s)) is probably not going to be acceptable due to its fragility. I have no good other suggestions, but I'm not eager to allow code in that weakens protection. >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -43,6 +43,7 @@ >>> #include <asm/page.h> >>> #include <asm/numa.h> >>> #include <asm/flushtlb.h> >>> +#include <asm/shadow.h> >> >>Breaking ARM? >>> #ifdef CONFIG_X86 >>> #include <asm/p2m.h> >>> #include <asm/setup.h> /* for highmem_start only */ >>> @@ -1660,6 +1661,8 @@ int assign_pages( >>> page_set_owner(&pg[i], d); >>> smp_wmb(); /* Domain pointer must be visible before updating >>> refcnt. >>*/ >>> pg[i].count_info = PGC_allocated | 1; >>> + if ( is_pv_domain(d) ) >>> + shadow_set_access(&pg[i], p2m_get_hostp2m(d)->default_access); >> >>I don't think you should call shadow code from here. > > Should I add a p2m wrapper for this, so that it is valid only for x86 and a > no-op for ARM? That's not necessarily enough, but at least presumably the right route: You also need to avoid fiddling with struct page_info fields that may be used (now or in the future) for other purposes, i.e. you need to gate the setting of the flags by more than just is_pv_domain(). >>> @@ -65,6 +67,11 @@ do { >> \ >>> case 8: __put_user_asm(x,ptr,retval,"q","","ir",errret);break; \ >>> default: __put_user_bad(); \ >>> } \ >>> + if ( unlikely(current->arch.pv_vcpu.need_cr0_wp_set) ) \ >>> + { \ >>> + write_cr0(read_cr0() | X86_CR0_WP); \ >>> + current->arch.pv_vcpu.need_cr0_wp_set = 0; \ >>> + } \ >>> } while (0) >> >>Please obey to the original indentation method. > > I thought tab characters were not allowed for indentation. But I guess you > do not want tabs and spaces for indentation purposes to be mixed within a > macro? Correct - ./CODING_STYLE explicitly says that you should match existing coding style when an entire file (or a significant code portion) was imported from elsewhere without style adjustment. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |