x86_emulate: split the {reg, mem} union in struct operand.
PATCH 1/1 x86_emulate: split the {reg, mem} union in struct operand.
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 <[email protected]>
CC Keir Fraser <[email protected]>
CC Jan Beulich <[email protected]>
CC Andrew Cooper <[email protected]>
--- xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
xen/arch/x86/x86_emulate/x86_emulate.c +5 -3
@@ -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 ;
Signature · 1 line
2.1.4
Quoted 7 lines · 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 <[email protected]>CC Keir Fraser <[email protected]>CC Jan Beulich <[email protected]>CC Andrew Cooper <[email protected]>
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
Quoted 43 lines
--- xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)xen/arch/x86/x86_emulate/x86_emulate.c +5
-3@@ -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 ;
Quoted 4 lines · Andrew Cooper
Quoted 3 lines · Tim Deegan
In the hopes of making any future errors along the lines of XSA-123 into clean crashes instead of memory corruption bugs.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Tim Deegan <[email protected]>CC Keir Fraser <[email protected]>CC Jan Beulich <[email protected]>CC Andrew Cooper <[email protected]>I am very much in favour of this change.
Does
REG_POISONwork with a 32bit compile of the test harness?
No it will not. v2 on its way...
Quoted 2 lines
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.
Quoted 44 lines
Quoted 43 lines
--- xen/arch/x86/x86_emulate/x86_emulate.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)xen/arch/x86/x86_emulate/x86_emulate.c +5
-3@@ -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 1/1 x86_emulate: split the {reg, mem} union in struct operand.
In reply to Tim DeeganIn 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 <[email protected]>
CC Keir Fraser <[email protected]>
CC Jan Beulich <[email protected]>
CC Andrew Cooper <[email protected]>
---
v2: tweak poison values (Andrew Cooper)
--- xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
xen/arch/x86/x86_emulate/x86_emulate.c +9 -3
@@ -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 ;
Signature · 1 line
2.1.4
Quoted 7 lines · 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 <[email protected]>CC Keir Fraser <[email protected]>CC Jan Beulich <[email protected]>CC Andrew Cooper <[email protected]>
Reviewed-by Andrew Cooper <[email protected]>
Quoted 49 lines
---
v2: tweak poison values (Andrew Cooper)--- xen/arch/x86/x86_emulate/x86_emulate.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)xen/arch/x86/x86_emulate/x86_emulate.c +9
-3@@ -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 ;
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 26.03.15 at 13:40, <[email protected]> wrote:
xen/arch/x86/x86_emulate/x86_emulate.c +0
-1@@ -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;
xen/arch/x86/x86_emulate/x86_emulate.c x86_emulate( · line 1463
Quoted 10 lines
xen/arch/x86/x86_emulate/x86_emulate.c +3
-1+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 1/1 x86_emulate: split the {reg, mem} union in struct operand.
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 <[email protected]>
Reviewed-by Andrew Cooper <[email protected]>
CC Keir Fraser <[email protected]>
CC Jan Beulich <[email protected]>
---
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(-)
xen/arch/x86/x86_emulate/x86_emulate.c +16 -11
@@ -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 ;
Signature · 1 line
2.1.0