Re: [Xen-devel] [PATCH 3/4] x86/vmx: Fix security issue when a guest balloons out the #VE info page

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.02.19 at 18:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 20/02/2019 14:37, Jan Beulich wrote:
>>>>> On 19.02.19 at 23:18, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -58,25 +57,67 @@ altp2m_vcpu_destroy(struct vcpu *v)
>>>  
>>>  int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn)
>>>  {
>>> +    struct domain *d = v->domain;
>>> +    struct altp2mvcpu *a = &vcpu_altp2m(v);
>>>      p2m_type_t p2mt;
>>> +    mfn_t mfn;
>>> +    struct page_info *pg;
>>> +    int rc;
>>> +
>>> +    /* Early exit path if #VE is already configured. */
>>> +    if ( a->veinfo_pg )
>>> +        return -EEXIST;
>>> +
>>> +    mfn = get_gfn(d, gfn_x(gfn), &p2mt);
>>> +
>>> +    /*
>>> +     * Looking for a plain piece of guest writeable RAM.  Take an extra 
> page
>>> +     * reference to reflect our intent to point the VMCS at it.
>>> +     */
>>> +    if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) ||
>>> +         p2m_is_readonly(p2mt) || !get_page(pg = mfn_to_page(mfn), d) )
>> p2m_is_ram() is pretty broad a class, and p2m_is_readonly() removes
>> only usefully p2m_ram_ro and p2m_ram_shared from the set. In
>> particular p2m_ioreq_server is thus permitted, as are all p2m_paging_*
>> which don't produce INVALID_MFN. I don't think that's what you want.
>> I also don't think you really want to exclude p2m_ram_logdirty - if
>> anything you might need to convert that type to p2m_ram_rw in case
>> you find the page in that state.
> 
> Flipping logdirty back to ram cannot reasonably be a caller requirement,

Sure - hence my "if anything".

> but you are right that logdirty pages ought to be eligible.  Holding
> extra references doesn't interact with logdirty operations.
> 
>> Can't you simply call check_get_page_from_gfn() here anyway?
>>
>> Furthermore I'm uncertain if get_page() is quite enough: Don't you
>> also want a writable type reference? It may not strictly be needed at
>> this point, but I think we're trying to make the distinction in new code,
>> like was e.g. done in recent Viridian changes.
> 
> No - I don't want to take a writeable reference.
> 
> Not because it is necessarily the wrong thing to do longterm, but
> because it is not the current behaviour, and is therefore substantially
> more risky in terms of subtle unexpected side effects (especially for
> 4.12 at this point).
> 
> We have years and years of XSAs demonstrating that things tend to go
> wrong.  It is irresponsible to start introducing a new reference
> counting scheme without doing the work to first justify the safety of
> change, and have a reasonable demonstration that the result is safe,
> e.g. with an IOMMU in the mix.

Well, afaict it has been a mix of both models basically from the
beginning. Whenever new code gets added to properly take
references, you have to decide anyway which of the two
possible variants to follow (as long as there is such an
inconsistency). I don't see it as particularly more or less risk to go
one route vs the other. I do agree though that we should settle
on one approach rather sooner than later, and then make all HVM
code behave that way.

Anyway - as long as we're convinced the writable ref is not strictly
needed, I'm not going to object to just acquiring a general ref
here. But as said, I'm not really certain there is no place where we
actually expect pages written to and owned by HVM guests to have
a writable ref.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel