* [PATCH 0/1] Fix for a buggy XSA-321 resolution in Xen 4.9 @ 2021-02-15 23:46 M. Vefa Bicakci 2021-02-15 23:46 ` [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport M. Vefa Bicakci 0 siblings, 1 reply; 5+ messages in thread From: M. Vefa Bicakci @ 2021-02-15 23:46 UTC (permalink / raw) To: xen-devel; +Cc: m.v.b, marmarek, roger.pau, jbeulich Hello all, I found a bug in Xen 4.9 involving what I think is an incorrectly backported patch for XSA-321. I realize that Xen 4.9 no longer receives security support, but as a Qubes OS 4.0 user, I still rely on Xen 4.8, and this bug affects me negatively. I would really appreciate it if anyone could review the following patch in this e-mail thread. I am mainly looking for a confirmation as to whether the patch fixes the buggy backport in an acceptable manner. The description of the patch is very (and possibly too) detailed and should have all of the necessary background. Thank you in advance, Vefa M. Vefa Bicakci (1): x86/ept: Fix buggy XSA-321 backport xen/arch/x86/mm/p2m-ept.c | 57 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 29 deletions(-) base-commit: 4597fc97b3b8870c39214e3aa4132ab711a40691 -- 2.29.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport 2021-02-15 23:46 [PATCH 0/1] Fix for a buggy XSA-321 resolution in Xen 4.9 M. Vefa Bicakci @ 2021-02-15 23:46 ` M. Vefa Bicakci 2021-02-16 9:20 ` Roger Pau Monné 0 siblings, 1 reply; 5+ messages in thread From: M. Vefa Bicakci @ 2021-02-15 23:46 UTC (permalink / raw) To: xen-devel; +Cc: m.v.b, marmarek, roger.pau, jbeulich 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. 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. The incorrect backport was detected with a software configuration consisting of Qubes OS 4.0 running Xen 4.8.5 in conjunction with Linux 5.10.y. Linux 5.10.y has the recently introduced "unpopulated-alloc" feature. The "unpopulated-alloc" feature uncovered the following error message in Xen's log buffer, whenever an HVM domain with PCI passthrough for a USB controller exposed an external USB SSD's block device to a PVH domain via xen-blkback/xen-blkfront, and whenever the PVH domain in question made rapid write accesses to the exposed block device: [VT-D]DMAR:[DMA Read] Request device [0000:00:14.0] fault addr 38600000, iommu reg = ... [VT-D]DMAR: reason 06 - PTE Read access is not set This was only observed when EPT page table sharing was enabled for the IOMMU. Upon debugging this issue further, it was noticed that Xen 4.12.0 had the same issue, but Xen 4.12.4 did not. Bisection led to the commit that fixed this issue, which was later discovered to not have been backported correctly to Xen 4.9. This patch has been tested with Xen 4.8 and Qubes OS 4.0. Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com> --- xen/arch/x86/mm/p2m-ept.c | 57 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 036771f43ccd..8492f4c1d321 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -48,7 +48,7 @@ static inline bool_t is_epte_valid(ept_entry_t *e) static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new, int level) { - int rc; + int rc = 0; unsigned long oldmfn = mfn_x(INVALID_MFN); bool_t check_foreign = (new.mfn != entryptr->mfn || new.sa_p2mt != entryptr->sa_p2mt); @@ -56,37 +56,39 @@ 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)) ) - { - rc = -EINVAL; - if ( !is_epte_present(&new) ) - goto out; - - if ( check_foreign ) + } else { + if ( unlikely(p2m_is_foreign(new.sa_p2mt)) ) { - struct domain *fdom; + rc = -EINVAL; + if ( !is_epte_present(&new) ) + goto out; - if ( !mfn_valid(_mfn(new.mfn)) ) - goto out; + if ( check_foreign ) + { + struct domain *fdom; - rc = -ESRCH; - fdom = page_get_owner(mfn_to_page(new.mfn)); - if ( fdom == NULL ) - goto out; + if ( !mfn_valid(_mfn(new.mfn)) ) + goto out; - /* get refcount on the page */ - rc = -EBUSY; - if ( !get_page(mfn_to_page(new.mfn), fdom) ) - 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; + if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && check_foreign ) + oldmfn = entryptr->mfn; + + if ( unlikely(oldmfn != mfn_x(INVALID_MFN)) ) + put_page(mfn_to_page(oldmfn)); + + } write_atomic(&entryptr->epte, new.epte); @@ -103,9 +105,6 @@ static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new, 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: -- 2.29.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport 2021-02-15 23:46 ` [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport M. Vefa Bicakci @ 2021-02-16 9:20 ` Roger Pau Monné 2021-02-16 12:48 ` M. Vefa Bicakci 0 siblings, 1 reply; 5+ messages in thread From: Roger Pau Monné @ 2021-02-16 9:20 UTC (permalink / raw) To: M. Vefa Bicakci; +Cc: xen-devel, marmarek, jbeulich 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) ) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport 2021-02-16 9:20 ` Roger Pau Monné @ 2021-02-16 12:48 ` M. Vefa Bicakci 2021-02-17 1:55 ` M. Vefa Bicakci 0 siblings, 1 reply; 5+ messages in thread From: M. Vefa Bicakci @ 2021-02-16 12:48 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, marmarek, jbeulich 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) ) > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport 2021-02-16 12:48 ` M. Vefa Bicakci @ 2021-02-17 1:55 ` M. Vefa Bicakci 0 siblings, 0 replies; 5+ messages in thread From: M. Vefa Bicakci @ 2021-02-17 1:55 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, marmarek, jbeulich On 16/02/2021 07.48, M. Vefa Bicakci wrote: > On 16/02/2021 04.20, Roger Pau Monné wrote: >> On Mon, Feb 15, 2021 at 06:46:19PM -0500, M. Vefa Bicakci wrote: > [snipped by Vefa] > >> 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. > [snipped by Vefa] > >> 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 Hello Roger, I have tested your patch, and I am happy to confirm that it too resolves the issue I have described in my original patch description. Thank you! When I find some more time, I would like to prepare a GitHub pull request for Qubes OS 4.0's version of Xen 4.8.5 with your patch so that other users do not encounter the same issue. I would like to properly credit your contribution. Would you be able to send a patch with a Signed-off-by tag in its description? Thanks again, 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) ) >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-17 1:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-15 23:46 [PATCH 0/1] Fix for a buggy XSA-321 resolution in Xen 4.9 M. Vefa Bicakci 2021-02-15 23:46 ` [PATCH 1/1] x86/ept: Fix buggy XSA-321 backport M. Vefa Bicakci 2021-02-16 9:20 ` Roger Pau Monné 2021-02-16 12:48 ` M. Vefa Bicakci 2021-02-17 1:55 ` M. Vefa Bicakci
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.