[PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas K Lengyel]

As pointed out by Jan Beulich in http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html: "Read-modify-write instructions absolutely need to be treated as read accesses, yet hardware doesn't guarantee to tell us so (they may surface as just write accesses)." This patch addresses the issue in both the VMX and the SVM side.

VMX: Treat all write data access violations also as read violations (in addition to those that were already reported as read violations).
SVM: Refine the meaning of read data access violations to distinguish between read/write and instruction fetch access violations.

With this patch both VMX and SVM specific nested page fault handling code reports violations the same way, thus abstracting the hardware specific behaviour from the layers above.

Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

---
v8: - Refine the comment in the SVM side.
v7: - Add comment blocks describing the rationale behind the specific handling of violations.
    - Tweak the logic in the VMX code to better match the description in the manual.
---
 xen/arch/x86/hvm/svm/svm.c |  7 ++++++-
 xen/arch/x86/hvm/vmx/vmx.c | 15 ++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1f72e19..e6317e2 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1403,8 +1403,13 @@ static void svm_do_nested_pgfault(struct vcpu *v,
     p2m_access_t p2ma;
     struct p2m_domain *p2m = NULL;
 
+    /*
+     * Since HW doesn't explicitly provide a read access bit and we need to
+     * somehow describe read-modify-write instructions we will conservatively
+     * set read_access for all memory accesses that are not instruction fetches.
+     */
     struct npfec npfec = {
-        .read_access = 1, /* All NPFs count as reads */
+        .read_access = !(pfec & PFEC_insn_fetch),
         .write_access = !!(pfec & PFEC_write_access),
         .insn_fetch = !!(pfec & PFEC_insn_fetch)
     };
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 656ce61..2a02e57 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2353,8 +2353,21 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     p2m_type_t p2mt;
     int ret;
     struct domain *d = current->domain;
+
+    /*
+     * We treat all write violations also as read violations.
+     * The reason why this is required is the following warning:
+     * "An EPT violation that occurs during as a result of execution of a
+     * read-modify-write operation sets bit 1 (data write). Whether it also
+     * sets bit 0 (data read) is implementation-specific and, for a given
+     * implementation, may differ for different kinds of read-modify-write
+     * operations."
+     * - Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+     *   Volume 3C: System Programming Guide, Part 3
+     */
     struct npfec npfec = {
-        .read_access = !!(qualification & EPT_READ_VIOLATION),
+        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
+                       !!(qualification & EPT_WRITE_VIOLATION),
         .write_access = !!(qualification & EPT_WRITE_VIOLATION),
         .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
     };
-- 
2.1.0.rc1

Re: [PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tim Deegan]

At 15:40 +0200 on 19 Aug (1408459219), Tamas K Lengyel wrote:
> As pointed out by Jan Beulich in http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html: "Read-modify-write instructions absolutely need to be treated as read accesses, yet hardware doesn't guarantee to tell us so (they may surface as just write accesses)." This patch addresses the issue in both the VMX and the SVM side.
> 
> VMX: Treat all write data access violations also as read violations (in addition to those that were already reported as read violations).
> SVM: Refine the meaning of read data access violations to distinguish between read/write and instruction fetch access violations.
> 
> With this patch both VMX and SVM specific nested page fault handling code reports violations the same way, thus abstracting the hardware specific behaviour from the layers above.
> 
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, August 28, 2014 3:14 AM
> 
> At 15:40 +0200 on 19 Aug (1408459219), Tamas K Lengyel wrote:
> > As pointed out by Jan Beulich in
> http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> "Read-modify-write instructions absolutely need to be treated as read
> accesses, yet hardware doesn't guarantee to tell us so (they may surface as
> just write accesses)." This patch addresses the issue in both the VMX and the
> SVM side.
> >
> > VMX: Treat all write data access violations also as read violations (in
> addition to those that were already reported as read violations).
> > SVM: Refine the meaning of read data access violations to distinguish
> between read/write and instruction fetch access violations.
> >
> > With this patch both VMX and SVM specific nested page fault handling code
> reports violations the same way, thus abstracting the hardware specific
> behaviour from the layers above.
> >
> > Suggested-by: Jan Beulich <JBeulich@suse.com>
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Reviewed-by: Tim Deegan <tim@xen.org>

As I commented earlier in another mail, I still think the right way is to
do the treatment in general place... 

Thanks
Kevin

Re: [PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tim Deegan]

At 21:40 +0000 on 28 Aug (1409258457), Tian, Kevin wrote:
> > From: Tim Deegan [mailto:tim@xen.org]
> > Sent: Thursday, August 28, 2014 3:14 AM
> > 
> > At 15:40 +0200 on 19 Aug (1408459219), Tamas K Lengyel wrote:
> > > As pointed out by Jan Beulich in
> > http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> > "Read-modify-write instructions absolutely need to be treated as read
> > accesses, yet hardware doesn't guarantee to tell us so (they may surface as
> > just write accesses)." This patch addresses the issue in both the VMX and the
> > SVM side.
> > >
> > > VMX: Treat all write data access violations also as read violations (in
> > addition to those that were already reported as read violations).
> > > SVM: Refine the meaning of read data access violations to distinguish
> > between read/write and instruction fetch access violations.
> > >
> > > With this patch both VMX and SVM specific nested page fault handling code
> > reports violations the same way, thus abstracting the hardware specific
> > behaviour from the layers above.
> > >
> > > Suggested-by: Jan Beulich <JBeulich@suse.com>
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > 
> > Reviewed-by: Tim Deegan <tim@xen.org>
> 
> As I commented earlier in another mail, I still think the right way is to
> do the treatment in general place... 

Understood; I guess it's clear that I disagree, but I guess I shuld
elaborate.  AIUI you're suggesting that we should adjust the consumers
of this field to handle the fact that r/m/w operations might be
reported as writes.  But that:
 - duplicates logic at a number of sites;
 - requires new consumers of the field to be aware of this wrinkle in
   the spec; and
 - makes changes to common code that are only needed on Intel hardware.

Whereas this patch just sorts the problem out once and for all, in one
place.  And as far as I can see it has no big disadvantages -- after
all now that we're aware of this behaviour, we have to treat all
writes as r/m/w anyway for safety.

Cheers,

Tim.

Re: [PATCH v8 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Friday, August 29, 2014 2:50 AM
> 
> At 21:40 +0000 on 28 Aug (1409258457), Tian, Kevin wrote:
> > > From: Tim Deegan [mailto:tim@xen.org]
> > > Sent: Thursday, August 28, 2014 3:14 AM
> > >
> > > At 15:40 +0200 on 19 Aug (1408459219), Tamas K Lengyel wrote:
> > > > As pointed out by Jan Beulich in
> > > http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> > > "Read-modify-write instructions absolutely need to be treated as read
> > > accesses, yet hardware doesn't guarantee to tell us so (they may surface
> as
> > > just write accesses)." This patch addresses the issue in both the VMX and
> the
> > > SVM side.
> > > >
> > > > VMX: Treat all write data access violations also as read violations (in
> > > addition to those that were already reported as read violations).
> > > > SVM: Refine the meaning of read data access violations to distinguish
> > > between read/write and instruction fetch access violations.
> > > >
> > > > With this patch both VMX and SVM specific nested page fault handling
> code
> > > reports violations the same way, thus abstracting the hardware specific
> > > behaviour from the layers above.
> > > >
> > > > Suggested-by: Jan Beulich <JBeulich@suse.com>
> > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > >
> > > Reviewed-by: Tim Deegan <tim@xen.org>
> >
> > As I commented earlier in another mail, I still think the right way is to
> > do the treatment in general place...
> 
> Understood; I guess it's clear that I disagree, but I guess I shuld
> elaborate.  AIUI you're suggesting that we should adjust the consumers
> of this field to handle the fact that r/m/w operations might be
> reported as writes.  But that:
>  - duplicates logic at a number of sites;
>  - requires new consumers of the field to be aware of this wrinkle in
>    the spec; and
>  - makes changes to common code that are only needed on Intel hardware.
> 
> Whereas this patch just sorts the problem out once and for all, in one
> place.  And as far as I can see it has no big disadvantages -- after
> all now that we're aware of this behaviour, we have to treat all
> writes as r/m/w anyway for safety.
> 

well, here both your argument and my argument are based on future
possibility: either concern on duplicating logic in other callers, or concern
on usages which wants a more accurate r/w statistics. Given that now
the read-modify-write is the only exploited problem, and all other guys
agree it's the right way to go, I'll give my ack here:

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin