[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
>>> On 25.01.19 at 12:10, <andrew.cooper3@xxxxxxxxxx> wrote: > On 25/01/2019 10:25, Jan Beulich wrote: >>>>> On 24.01.19 at 19:28, <andrew.cooper3@xxxxxxxxxx> wrote: >>> Code clearing the "Suppress VE" bit in an EPT entry isn't nececsserily >>> running >>> in current context. In ALTP2M_external mode, it definitely is not, and in >>> PV >>> context, vcpu_altp2m(current) acts upon the HVM union. >>> >>> Even if we could sensibly resolve the target vCPU, it may legitimately not >>> be >>> fully set up at this point, so rejecting the EPT modification would be >>> buggy. >>> >>> There is a path in hvm_hap_nested_page_fault() which explicitly emulates #VE >>> in the cpu_has_vmx_virt_exceptions case, so the -EOPNOTSUPP part of this >>> condition is also wrong. >>>[...] >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -702,16 +702,6 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, >>> mfn_t mfn, >>> >>> ASSERT(ept); >>> >>> - if ( !sve ) >>> - { >>> - if ( !cpu_has_vmx_virt_exceptions ) >>> - return -EOPNOTSUPP; >>> - >>> - /* #VE should be enabled for this vcpu. */ >>> - if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) ) >>> - return -ENXIO; >>> - } >> How about retaining the latter, but qualifying it with >> current->domain == d? > > Why? There is a paragraph in the commit message explaining why this > check is wrong even when it isn't an out-of-bounds read. I'm struggling. For clarity I've retained all of the relevant parts of the description in context above. I can't identify where you talk about an out of bounds read there. So - for the first paragraph, acting upon the wrong side of the union does not apply with the added qualifier, - for the second paragraph, the domain itself requesting an action upon something not fully initialized yet looks to deserve an error return; it looks wrong to me to request #VE without first setting up the page needed for its proper delivery (I'd even question the validity of doing so by a controlling domain, but I accept that we can't work out the correct vCPU to check - perhaps the right thing would be to check all of them, as the P2M is per-domain, not per-vCPU), - the third paragraph is about the other condition, which I agree should go away. I'm sorry for being dense, but I seem to need a more explicit explanation. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |