[PATCH] x86_emulate: split the {reg, mem} union in struct operand.

[PATCH] x86_emulate: split the {reg, mem} union in struct operand.

[Tim Deegan]

In the hopes of making any future errors along the lines of XSA-123
into clean crashes instead of memory corruption bugs.

Signed-off-by: Tim Deegan <tim@xen.org>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c082c9e..37d1fea 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -319,7 +319,7 @@ struct operand {
     /* Original operand value. */
     unsigned long orig_val;
 
-    union {
+    struct {
         /* OP_REG: Pointer to register field. */
         unsigned long *reg;
         /* OP_MEM: Segment and offset. */
@@ -329,6 +329,7 @@ struct operand {
         } mem;
     };
 };
+#define REG_POISON ((unsigned long *) 0x8000000000008086UL) /* non-canonical */
 
 typedef union {
     uint64_t mmx;
@@ -1447,14 +1448,15 @@ x86_emulate(
     unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
     bool_t lock_prefix = 0;
     int override_seg = -1, rc = X86EMUL_OKAY;
-    struct operand src, dst;
+    struct operand src = { .reg = REG_POISON };
+    struct operand dst = { .reg = REG_POISON };
     enum x86_swint_type swint_type;
     DECLARE_ALIGNED(mmval_t, mmval);
     /*
      * Data operand effective address (usually computed from ModRM).
      * Default is a memory operand relative to segment DS.
      */
-    struct operand ea = { .type = OP_MEM };
+    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
     ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
 
     ctxt->retire.byte = 0;
-- 
2.1.4

Re: [PATCH] x86_emulate: split the {reg, mem} union in struct operand.

[Andrew Cooper]

On 26/03/15 12:16, Tim Deegan wrote:
> In the hopes of making any future errors along the lines of XSA-123
> into clean crashes instead of memory corruption bugs.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>

I am very much in favour of this change.

Does REG_POISON work with a 32bit compile of the test harness?

Independent of this, it might be easier to spot in crashes if the upper 
word of the address was also 8086

~Andrew

> ---
>   xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c082c9e..37d1fea 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -319,7 +319,7 @@ struct operand {
>       /* Original operand value. */
>       unsigned long orig_val;
>   
> -    union {
> +    struct {
>           /* OP_REG: Pointer to register field. */
>           unsigned long *reg;
>           /* OP_MEM: Segment and offset. */
> @@ -329,6 +329,7 @@ struct operand {
>           } mem;
>       };
>   };
> +#define REG_POISON ((unsigned long *) 0x8000000000008086UL) /* non-canonical */
>   
>   typedef union {
>       uint64_t mmx;
> @@ -1447,14 +1448,15 @@ x86_emulate(
>       unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
>       bool_t lock_prefix = 0;
>       int override_seg = -1, rc = X86EMUL_OKAY;
> -    struct operand src, dst;
> +    struct operand src = { .reg = REG_POISON };
> +    struct operand dst = { .reg = REG_POISON };
>       enum x86_swint_type swint_type;
>       DECLARE_ALIGNED(mmval_t, mmval);
>       /*
>        * Data operand effective address (usually computed from ModRM).
>        * Default is a memory operand relative to segment DS.
>        */
> -    struct operand ea = { .type = OP_MEM };
> +    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
>       ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
>   
>       ctxt->retire.byte = 0;

Re: [PATCH] x86_emulate: split the {reg, mem} union in struct operand.

[Tim Deegan]

At 12:25 +0000 on 26 Mar (1427372755), Andrew Cooper wrote:
> On 26/03/15 12:16, Tim Deegan wrote:
> > In the hopes of making any future errors along the lines of XSA-123
> > into clean crashes instead of memory corruption bugs.
> >
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I am very much in favour of this change.
> 
> Does REG_POISON work with a 32bit compile of the test harness?

No it will not.  v2 on its way...

> Independent of this, it might be easier to spot in crashes if the upper 
> word of the address was also 8086

Sure, why not?

Tim.

> > ---
> >   xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> > index c082c9e..37d1fea 100644
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -319,7 +319,7 @@ struct operand {
> >       /* Original operand value. */
> >       unsigned long orig_val;
> >   
> > -    union {
> > +    struct {
> >           /* OP_REG: Pointer to register field. */
> >           unsigned long *reg;
> >           /* OP_MEM: Segment and offset. */
> > @@ -329,6 +329,7 @@ struct operand {
> >           } mem;
> >       };
> >   };
> > +#define REG_POISON ((unsigned long *) 0x8000000000008086UL) /* non-canonical */
> >   
> >   typedef union {
> >       uint64_t mmx;
> > @@ -1447,14 +1448,15 @@ x86_emulate(
> >       unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
> >       bool_t lock_prefix = 0;
> >       int override_seg = -1, rc = X86EMUL_OKAY;
> > -    struct operand src, dst;
> > +    struct operand src = { .reg = REG_POISON };
> > +    struct operand dst = { .reg = REG_POISON };
> >       enum x86_swint_type swint_type;
> >       DECLARE_ALIGNED(mmval_t, mmval);
> >       /*
> >        * Data operand effective address (usually computed from ModRM).
> >        * Default is a memory operand relative to segment DS.
> >        */
> > -    struct operand ea = { .type = OP_MEM };
> > +    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
> >       ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
> >   
> >       ctxt->retire.byte = 0;
> 

[PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.

[Tim Deegan]

In the hopes of making any future errors along the lines of XSA-123
into clean crashes instead of memory corruption bugs.

Signed-off-by: Tim Deegan <tim@xen.org>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: tweak poison values (Andrew Cooper)
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c082c9e..9dbf063 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -319,7 +319,7 @@ struct operand {
     /* Original operand value. */
     unsigned long orig_val;
 
-    union {
+    struct {
         /* OP_REG: Pointer to register field. */
         unsigned long *reg;
         /* OP_MEM: Segment and offset. */
@@ -329,6 +329,11 @@ struct operand {
         } mem;
     };
 };
+#ifdef __x86_64__
+#define REG_POISON ((unsigned long *) 0x8086000000008086UL) /* non-canonical */
+#else
+#define REG_POISON NULL /* 32-bit builds are for user-space, so NULL is OK. */
+#endif
 
 typedef union {
     uint64_t mmx;
@@ -1447,14 +1452,15 @@ x86_emulate(
     unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
     bool_t lock_prefix = 0;
     int override_seg = -1, rc = X86EMUL_OKAY;
-    struct operand src, dst;
+    struct operand src = { .reg = REG_POISON };
+    struct operand dst = { .reg = REG_POISON };
     enum x86_swint_type swint_type;
     DECLARE_ALIGNED(mmval_t, mmval);
     /*
      * Data operand effective address (usually computed from ModRM).
      * Default is a memory operand relative to segment DS.
      */
-    struct operand ea = { .type = OP_MEM };
+    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
     ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
 
     ctxt->retire.byte = 0;
-- 
2.1.4

Re: [PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.

[Andrew Cooper]

On 26/03/15 12:40, Tim Deegan wrote:
> In the hopes of making any future errors along the lines of XSA-123
> into clean crashes instead of memory corruption bugs.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v2: tweak poison values (Andrew Cooper)
> ---
>   xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index c082c9e..9dbf063 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -319,7 +319,7 @@ struct operand {
>       /* Original operand value. */
>       unsigned long orig_val;
>   
> -    union {
> +    struct {
>           /* OP_REG: Pointer to register field. */
>           unsigned long *reg;
>           /* OP_MEM: Segment and offset. */
> @@ -329,6 +329,11 @@ struct operand {
>           } mem;
>       };
>   };
> +#ifdef __x86_64__
> +#define REG_POISON ((unsigned long *) 0x8086000000008086UL) /* non-canonical */
> +#else
> +#define REG_POISON NULL /* 32-bit builds are for user-space, so NULL is OK. */
> +#endif
>   
>   typedef union {
>       uint64_t mmx;
> @@ -1447,14 +1452,15 @@ x86_emulate(
>       unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
>       bool_t lock_prefix = 0;
>       int override_seg = -1, rc = X86EMUL_OKAY;
> -    struct operand src, dst;
> +    struct operand src = { .reg = REG_POISON };
> +    struct operand dst = { .reg = REG_POISON };
>       enum x86_swint_type swint_type;
>       DECLARE_ALIGNED(mmval_t, mmval);
>       /*
>        * Data operand effective address (usually computed from ModRM).
>        * Default is a memory operand relative to segment DS.
>        */
> -    struct operand ea = { .type = OP_MEM };
> +    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
>       ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
>   
>       ctxt->retire.byte = 0;

Re: [PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.

[Jan Beulich]

>>> On 26.03.15 at 13:40, <tim@xen.org> wrote:
> @@ -1447,14 +1452,15 @@ x86_emulate(
>      unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
>      bool_t lock_prefix = 0;
>      int override_seg = -1, rc = X86EMUL_OKAY;
> -    struct operand src, dst;
> +    struct operand src = { .reg = REG_POISON };
> +    struct operand dst = { .reg = REG_POISON };
>      enum x86_swint_type swint_type;
>      DECLARE_ALIGNED(mmval_t, mmval);
>      /*
>       * Data operand effective address (usually computed from ModRM).
>       * Default is a memory operand relative to segment DS.
>       */
> -    struct operand ea = { .type = OP_MEM };
> +    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };

This failed my pre-push build check - older gcc  (4.3.4 in this case)
doesn't allow initializing fields of unnamed struct/union.

Jan

[PATCH v3] x86_emulate: split the {reg, mem} union in struct operand.

[Tim Deegan]

In the hopes of making any future errors along the lines of XSA-123
into clean crashes instead of memory corruption bugs.

Signed-off-by: Tim Deegan <tim@xen.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
---
v3: drop unhelpful unnamed struct (Jan Beulich / gcc)
v2: tweak poison values (Andrew Cooper)
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c082c9e..ae32c82 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -319,16 +319,20 @@ struct operand {
     /* Original operand value. */
     unsigned long orig_val;
 
-    union {
-        /* OP_REG: Pointer to register field. */
-        unsigned long *reg;
-        /* OP_MEM: Segment and offset. */
-        struct {
-            enum x86_segment seg;
-            unsigned long    off;
-        } mem;
-    };
+    /* OP_REG: Pointer to register field. */
+    unsigned long *reg;
+
+    /* OP_MEM: Segment and offset. */
+    struct {
+        enum x86_segment seg;
+        unsigned long    off;
+    } mem;
 };
+#ifdef __x86_64__
+#define REG_POISON ((unsigned long *) 0x8086000000008086UL) /* non-canonical */
+#else
+#define REG_POISON NULL /* 32-bit builds are for user-space, so NULL is OK. */
+#endif
 
 typedef union {
     uint64_t mmx;
@@ -1447,14 +1451,15 @@ x86_emulate(
     unsigned int op_bytes, def_op_bytes, ad_bytes, def_ad_bytes;
     bool_t lock_prefix = 0;
     int override_seg = -1, rc = X86EMUL_OKAY;
-    struct operand src, dst;
+    struct operand src = { .reg = REG_POISON };
+    struct operand dst = { .reg = REG_POISON };
     enum x86_swint_type swint_type;
     DECLARE_ALIGNED(mmval_t, mmval);
     /*
      * Data operand effective address (usually computed from ModRM).
      * Default is a memory operand relative to segment DS.
      */
-    struct operand ea = { .type = OP_MEM };
+    struct operand ea = { .type = OP_MEM, .reg = REG_POISON };
     ea.mem.seg = x86_seg_ds; /* gcc may reject anon union initializer */
 
     ctxt->retire.byte = 0;
-- 
2.1.0