[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport
On 16/02/2021 04.20, Roger Pau Monné wrote:
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.
Thank you for the feedback; this is a good point.
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?
Thank you! I will test your patch later today, and I will report
back by tomorrow.
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.
Ah, but my patch moves the call to put_page to a location above the
call to iommu_sync_cache, to make the code a bit similar to the same
function in Xen 4.14. This may not be necessary, though. This goes back
to your aforementioned point about having two separate patches, as my
patch does not make the move of the call to put_page obvious. In any
case, let's focus on your patch.
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.
I will test this. Thanks again! I really appreciate that you have
have taken the time and effort.
Vefa
---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) )
|
