Re: [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 15, 2021 at 06:46:19PM -0500, M. Vefa Bicakci wrote:
> This commit aims to fix commit a852040fe3ab ("x86/ept: flush cache when
> modifying PTEs and sharing page tables"). The aforementioned commit is
> for the stable-4.9 branch of Xen and is a backported version of commit
> c23274fd0412 ("x86/ept: flush cache when modifying PTEs and sharing page
> tables"), which was for Xen 4.14.0-rc5 and which fixes the security
> issue reported by XSA-321.
> 
> Prior to the latter commit, the function atomic_write_ept_entry in Xen
> 4.14.y consisted mostly of a call to p2m_entry_modify followed by an
> atomic replacement of a page table entry, and the latter commit adds
> a call to iommu_sync_cache for a specific condition:
> 
>    static int atomic_write_ept_entry(struct p2m_domain *p2m,
>                                      ept_entry_t *entryptr, ept_entry_t new,
>                                      int level)
>    {
>        int rc = p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt,
>                                  _mfn(new.mfn), _mfn(entryptr->mfn), level + 
> 1);
> 
>        if ( rc )
>            return rc;
> 
>        write_atomic(&entryptr->epte, new.epte);
> 
>   +    /* snipped comment block */
>   +    if ( !new.recalc && iommu_use_hap_pt(p2m->domain) )
>   +        iommu_sync_cache(entryptr, sizeof(*entryptr));
>   +
>        return 0;
>    }
> 
> However, the backport to Xen 4.9.y is a bit different because
> atomic_write_ept_entry does not consist solely of a call to
> p2m_entry_modify, which does not exist in Xen 4.9.y. I am quoting from
> Xen 4.8.y for convenience:
> 
>    static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
>                                      int level)
>    {
>        int rc;
>        unsigned long oldmfn = mfn_x(INVALID_MFN);
>        bool_t check_foreign = (new.mfn != entryptr->mfn ||
>                                new.sa_p2mt != entryptr->sa_p2mt);
> 
>        if ( level )
>        {
>            ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
>            write_atomic(&entryptr->epte, new.epte);
>            return 0;
>        }
> 
>        if ( unlikely(p2m_is_foreign(new.sa_p2mt)) )
>        {
>            rc = -EINVAL;
>            if ( !is_epte_present(&new) )
>                    goto out;
> 
>            if ( check_foreign )
>            {
>                struct domain *fdom;
> 
>                if ( !mfn_valid(new.mfn) )
>                    goto out;
> 
>                rc = -ESRCH;
>                fdom = page_get_owner(mfn_to_page(new.mfn));
>                if ( fdom == NULL )
>                    goto out;
> 
>                /* get refcount on the page */
>                rc = -EBUSY;
>                if ( !get_page(mfn_to_page(new.mfn), fdom) )
>                    goto out;
>            }
>        }
> 
>        if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && check_foreign )
>            oldmfn = entryptr->mfn;
> 
>        write_atomic(&entryptr->epte, new.epte);
> 
>   +    /* snipped comment block */
>   +    if ( !new.recalc && iommu_hap_pt_share )
>   +        iommu_sync_cache(entryptr, sizeof(*entryptr));
>   +
>        if ( unlikely(oldmfn != mfn_x(INVALID_MFN)) )
>            put_page(mfn_to_page(oldmfn));
> 
>        rc = 0;
> 
>     out:
>        if ( rc )
>            gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
>                     entryptr->epte, new.epte, rc);
>        return rc;
>    }
> 
> Based on inspection of the p2m_entry_modify function in Xen 4.14.1, it
> appears that the part of atomic_write_ept_entry above the call to
> write_atomic is encapsulated by p2m_entry_modify, which uncovers one
> issue with the backport: In Xen 4.14, if p2m_entry_modify returns early
> without an error, then the calls to write_atomic and iommu_sync_cache
> will still occur (assuming that the corresponding if condition is
> satisfied), whereas in Xen 4.9.y, there is a code path that can skip the
> call to iommu_sync_cache, in case the variable level is not zero:
> 
>   if ( level )
>   {
>      ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
>      write_atomic(&entryptr->epte, new.epte);
>      return 0;
>   }
> 
> This patch reorganizes the atomic_write_ept_entry to ensure that the
> call to iommu_sync_cache is not inadvertently skipped.

IMO this is likely too much change in a single patch, iff we really
wanted to do this you should have a pre-patch that re-arranges the
code without any functional change followed by a patch that fixes the
issue.

In any case I think this is too much change, so I would go for a
smaller fix like my proposal below. Can you please test it?

> Furthermore, in Xen 4.14.1, p2m_entry_modify calls
> 
>   put_page(mfn_to_page(oldmfn));
> 
> before the potential call to iommu_sync_cache in atomic_write_ept_entry.
> I am not sufficiently familiar with Xen to determine if this is a
> significant behavioural change, but this patch makes Xen 4.9.y similar
> to Xen 4.14.1 in that regard as well, by further re-organizing the code
> in atomic_write_ept_entry.

Well, that put_page is only relevant to PVH dom0, but you shouldn't
remove it. The put_page call in newer versions has been moved by
commit ce0224bf96a1a1f82 into p2m_entry_modify.

Here is my proposed fix, I think we could even do away with the else
branch, but if level is != 0 p2m_is_foreign should be false, so we
avoid an extra check.

Thanks, Roger.
---8<---
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 036771f43c..086739ffdd 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -56,11 +56,8 @@ static int atomic_write_ept_entry(ept_entry_t *entryptr, 
ept_entry_t new,
     if ( level )
     {
         ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
-        write_atomic(&entryptr->epte, new.epte);
-        return 0;
     }
-
-    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) )
+    else if ( unlikely(p2m_is_foreign(new.sa_p2mt)) )
     {
         rc = -EINVAL;
         if ( !is_epte_present(&new) )