* [PATCH] x86_emulate: split the {reg, mem} union in struct operand.
@ 2015-03-26 12:16 Tim Deegan
2015-03-26 12:25 ` Andrew Cooper
2015-03-26 12:40 ` [PATCH v2] " Tim Deegan
0 siblings, 2 replies; 7+ messages in thread
From: Tim Deegan @ 2015-03-26 12:16 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-26 12:16 [PATCH] x86_emulate: split the {reg, mem} union in struct operand Tim Deegan
@ 2015-03-26 12:25 ` Andrew Cooper
2015-03-26 12:34 ` Tim Deegan
2015-03-26 12:40 ` [PATCH v2] " Tim Deegan
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2015-03-26 12:25 UTC (permalink / raw)
To: Tim Deegan, xen-devel; +Cc: Keir Fraser, Jan Beulich
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-26 12:25 ` Andrew Cooper
@ 2015-03-26 12:34 ` Tim Deegan
0 siblings, 0 replies; 7+ messages in thread
From: Tim Deegan @ 2015-03-26 12:34 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Keir Fraser, Jan Beulich, xen-devel
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-26 12:16 [PATCH] x86_emulate: split the {reg, mem} union in struct operand Tim Deegan
2015-03-26 12:25 ` Andrew Cooper
@ 2015-03-26 12:40 ` Tim Deegan
2015-03-26 12:42 ` Andrew Cooper
2015-03-27 14:31 ` Jan Beulich
1 sibling, 2 replies; 7+ messages in thread
From: Tim Deegan @ 2015-03-26 12:40 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-26 12:40 ` [PATCH v2] " Tim Deegan
@ 2015-03-26 12:42 ` Andrew Cooper
2015-03-27 14:31 ` Jan Beulich
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2015-03-26 12:42 UTC (permalink / raw)
To: Tim Deegan, xen-devel; +Cc: Keir Fraser, Jan Beulich
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-26 12:40 ` [PATCH v2] " Tim Deegan
2015-03-26 12:42 ` Andrew Cooper
@ 2015-03-27 14:31 ` Jan Beulich
2015-03-27 14:44 ` [PATCH v3] " Tim Deegan
1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-03-27 14:31 UTC (permalink / raw)
To: Tim Deegan; +Cc: Andrew Cooper, Keir Fraser, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] x86_emulate: split the {reg, mem} union in struct operand.
2015-03-27 14:31 ` Jan Beulich
@ 2015-03-27 14:44 ` Tim Deegan
0 siblings, 0 replies; 7+ messages in thread
From: Tim Deegan @ 2015-03-27 14:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-27 14:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-26 12:16 [PATCH] x86_emulate: split the {reg, mem} union in struct operand Tim Deegan
2015-03-26 12:25 ` Andrew Cooper
2015-03-26 12:34 ` Tim Deegan
2015-03-26 12:40 ` [PATCH v2] " Tim Deegan
2015-03-26 12:42 ` Andrew Cooper
2015-03-27 14:31 ` Jan Beulich
2015-03-27 14:44 ` [PATCH v3] " Tim Deegan
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.