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

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.

> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }
>  
> -    if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
> -         mfn_eq(get_gfn_query_unlocked(v->domain, gfn_x(gfn), &p2mt),
> -                INVALID_MFN) )
> -        return -EINVAL;
> +    /*
> +     * Update veinfo_pg, making sure to be safe with concurrent hypercalls.
> +     * The first caller to make veinfo_pg become non-NULL will program its 
> MFN
> +     * into the VMCS, so must not be clobbered.  Callers which lose the race
> +     * back off with -EEXIST.
> +     */
> +    if ( cmpxchg(&a->veinfo_pg, NULL, pg) != NULL )
> +    {
> +        put_page(pg);
> +        rc = -EEXIST;
> +        goto out;
> +    }
>  
> -    vcpu_altp2m(v).veinfo_gfn = gfn;
> +    rc = 0;
>      altp2m_vcpu_update_vmfunc_ve(v);
>  
> -    return 0;
> + out:
> +    put_gfn(d, gfn_x(gfn));

Is there any reason why you need to hold the GFN ref until here? It would
seem to me that it can be dropped the moment you've obtained the general
page ref.

Jan



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