[PATCH for-4.11] x86/traps: Fix error handling of the pv %dr7 shadow state

[PATCH for-4.11] x86/traps: Fix error handling of the pv %dr7 shadow state

[Andrew Cooper]

c/s "x86/pv: Introduce and use x86emul_write_dr()" fixed a bug with IO shadow
handling, in that it remained stale and visible until %dr7.L/G got set again.

However, it neglected the -EPERM return inbetween these two hunks, introducing
a different bug in which a write to %dr7 which tries to set IO breakpoints
without %cr4.DE being set clobbers the IO state, rather than leaves it alone.

Instead, move the zeroing slightly later, which guarentees that the shadow
gets written exactly once, on a successful update to %dr7.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Juergen Gross <jgross@suse.com>

Although minor, this is a regression from Xen 4.10, so should be considered
for 4.11 at this point.  Given that PV debugging was basically completely
broken before the XSA-263 investigation work, the risk of further issues is
very small.
---
 xen/arch/x86/traps.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 8a99174..e79ca88 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2123,9 +2123,6 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( value & DR_GENERAL_DETECT )
             return -EPERM;
 
-        /* Zero the IO shadow before recalculating the real %dr7 */
-        v->arch.debugreg[5] = 0;
-
         /* DR7.{G,L}E = 0 => debugging disabled for this domain. */
         if ( value & DR7_ACTIVE_MASK )
         {
@@ -2154,6 +2151,10 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
                  !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
                 activate_debugregs(v);
         }
+        else
+            /* Zero the emulated controls if %dr7 isn't active. */
+            v->arch.debugreg[5] = 0;
+
         if ( v == curr )
             write_debugreg(7, value);
         break;
-- 
2.1.4


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

Re: [PATCH for-4.11] x86/traps: Fix error handling of the pv %dr7 shadow state

[Andrew Cooper]

On 01/06/18 15:06, Andrew Cooper wrote:
> c/s "x86/pv: Introduce and use x86emul_write_dr()" fixed a bug with IO shadow
> handling, in that it remained stale and visible until %dr7.L/G got set again.
>
> However, it neglected the -EPERM return inbetween these two hunks, introducing
> a different bug in which a write to %dr7 which tries to set IO breakpoints
> without %cr4.DE being set clobbers the IO state, rather than leaves it alone.
>
> Instead, move the zeroing slightly later, which guarentees that the shadow
> gets written exactly once, on a successful update to %dr7.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Juergen Gross <jgross@suse.com>
>
> Although minor, this is a regression from Xen 4.10, so should be considered
> for 4.11 at this point.  Given that PV debugging was basically completely
> broken before the XSA-263 investigation work, the risk of further issues is
> very small.

Sorry - this is a bad way of phrasing what I meant to say.  The
behaviour of 4.11 is less bad than 4.10, but still wrong (and in a way
which is technically a regression from 4.10).

~Andrew

> ---
>  xen/arch/x86/traps.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 8a99174..e79ca88 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2123,9 +2123,6 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>          if ( value & DR_GENERAL_DETECT )
>              return -EPERM;
>  
> -        /* Zero the IO shadow before recalculating the real %dr7 */
> -        v->arch.debugreg[5] = 0;
> -
>          /* DR7.{G,L}E = 0 => debugging disabled for this domain. */
>          if ( value & DR7_ACTIVE_MASK )
>          {
> @@ -2154,6 +2151,10 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>                   !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
>                  activate_debugregs(v);
>          }
> +        else
> +            /* Zero the emulated controls if %dr7 isn't active. */
> +            v->arch.debugreg[5] = 0;
> +
>          if ( v == curr )
>              write_debugreg(7, value);
>          break;


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

Re: [PATCH for-4.11] x86/traps: Fix error handling of the pv %dr7 shadow state

[Jan Beulich]

>>> On 01.06.18 at 16:06, <andrew.cooper3@citrix.com> wrote:
> c/s "x86/pv: Introduce and use x86emul_write_dr()" fixed a bug with IO shadow
> handling, in that it remained stale and visible until %dr7.L/G got set again.
> 
> However, it neglected the -EPERM return inbetween these two hunks, introducing
> a different bug in which a write to %dr7 which tries to set IO breakpoints
> without %cr4.DE being set clobbers the IO state, rather than leaves it alone.
> 
> Instead, move the zeroing slightly later, which guarentees that the shadow
> gets written exactly once, on a successful update to %dr7.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH for-4.11] x86/traps: Fix error handling of the pv %dr7 shadow state

[Juergen Gross]

On 01/06/18 16:06, Andrew Cooper wrote:
> c/s "x86/pv: Introduce and use x86emul_write_dr()" fixed a bug with IO shadow
> handling, in that it remained stale and visible until %dr7.L/G got set again.
> 
> However, it neglected the -EPERM return inbetween these two hunks, introducing
> a different bug in which a write to %dr7 which tries to set IO breakpoints
> without %cr4.DE being set clobbers the IO state, rather than leaves it alone.
> 
> Instead, move the zeroing slightly later, which guarentees that the shadow
> gets written exactly once, on a successful update to %dr7.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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