Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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