* Re: [PATCH] x86emul/fuzz: add a state sanitization function
2019-03-29 14:54 [PATCH] x86emul/fuzz: add a state sanitization function Jan Beulich
@ 2019-03-29 15:10 ` George Dunlap
2019-03-29 15:25 ` Jan Beulich
2019-04-01 7:46 ` [PATCH v2] " Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: George Dunlap @ 2019-03-29 15:10 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monne, Wei Liu, George Dunlap, Andrew Cooper
> On Mar 29, 2019, at 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>
> This is to accompany sanitize_input(). Just like for initial state we
> want to have state between two emulated insns sane, at least as far as
> assumptions in the main emulator go. Do minimal checking after segment
> register, CR, and MSR writes, and roll back to the old value in case of
> failure (raising #GP(0) at the same time).
>
> In the particular case observed, a CR0 write clearing CR0.PE was
> followed by a VEX-encoded insn, which the decoder accepts based on
> guest address size, restricting things just outside of the 64-bit case
> (real and virtual modes don't allow VEX-encoded insns). Subsequently
> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -76,6 +76,8 @@ static inline bool input_read(struct fuz
> return true;
> }
>
> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt);
> +
> static const char* const x86emul_return_string[] = {
> [X86EMUL_OKAY] = "X86EMUL_OKAY",
> [X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
> @@ -424,8 +426,19 @@ static int fuzz_write_segment(
> rc = maybe_fail(ctxt, "write_segment", true);
>
> if ( rc == X86EMUL_OKAY )
> + {
> c->segments[seg] = *reg;
>
> + if ( !sanitize_state(ctxt) )
> + {
> + struct segment_register old = c->segments[seg];
I think you have this in the wrong place.
Everything else looks good.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] x86emul/fuzz: add a state sanitization function
2019-03-29 15:10 ` George Dunlap
@ 2019-03-29 15:25 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2019-03-29 15:25 UTC (permalink / raw)
To: george.dunlap; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 29.03.19 at 16:10, <George.Dunlap@citrix.com> wrote:
>> On Mar 29, 2019, at 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> @@ -424,8 +426,19 @@ static int fuzz_write_segment(
>> rc = maybe_fail(ctxt, "write_segment", true);
>>
>> if ( rc == X86EMUL_OKAY )
>> + {
>> c->segments[seg] = *reg;
>>
>> + if ( !sanitize_state(ctxt) )
>> + {
>> + struct segment_register old = c->segments[seg];
>
> I think you have this in the wrong place.
Ouch. Thanks for noticing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] x86emul/fuzz: add a state sanitization function
2019-03-29 14:54 [PATCH] x86emul/fuzz: add a state sanitization function Jan Beulich
2019-03-29 15:10 ` George Dunlap
@ 2019-04-01 7:46 ` Jan Beulich
2019-04-01 10:44 ` George Dunlap
2019-04-02 13:01 ` [PATCH v3] x86emul/fuzz: add a state sanity checking function Jan Beulich
[not found] ` <5CA35D1302000000001041AA@prv1-mh.provo.novell.com>
3 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-04-01 7:46 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
This is to accompany sanitize_input(). Just like for initial state we
want to have state between two emulated insns sane, at least as far as
assumptions in the main emulator go. Do minimal checking after segment
register, CR, and MSR writes, and roll back to the old value in case of
failure (raising #GP(0) at the same time).
In the particular case observed, a CR0 write clearing CR0.PE was
followed by a VEX-encoded insn, which the decoder accepts based on
guest address size, restricting things just outside of the 64-bit case
(real and virtual modes don't allow VEX-encoded insns). Subsequently
_get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
clear) when trying to invoke YMM, ZMM, or OPMASK state.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Correct placement of new declaration in fuzz_write_segment().
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -76,6 +76,8 @@ static inline bool input_read(struct fuz
return true;
}
+static bool sanitize_state(struct x86_emulate_ctxt *ctxt);
+
static const char* const x86emul_return_string[] = {
[X86EMUL_OKAY] = "X86EMUL_OKAY",
[X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
@@ -424,8 +426,19 @@ static int fuzz_write_segment(
rc = maybe_fail(ctxt, "write_segment", true);
if ( rc == X86EMUL_OKAY )
+ {
+ struct segment_register old = c->segments[seg];
+
c->segments[seg] = *reg;
+ if ( !sanitize_state(ctxt) )
+ {
+ c->segments[seg] = old;
+ x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
+ rc = X86EMUL_EXCEPTION;
+ }
+ }
+
return rc;
}
@@ -452,6 +465,7 @@ static int fuzz_write_cr(
{
struct fuzz_state *s = ctxt->data;
struct fuzz_corpus *c = s->corpus;
+ unsigned long old;
int rc;
if ( reg >= ARRAY_SIZE(c->cr) )
@@ -461,9 +475,17 @@ static int fuzz_write_cr(
if ( rc != X86EMUL_OKAY )
return rc;
+ old = c->cr[reg];
c->cr[reg] = val;
- return X86EMUL_OKAY;
+ if ( !sanitize_state(ctxt) )
+ {
+ c->cr[reg] = old;
+ x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
+ rc = X86EMUL_EXCEPTION;
+ }
+
+ return rc;
}
#define fuzz_read_xcr emul_test_read_xcr
@@ -561,7 +583,16 @@ static int fuzz_write_msr(
{
if ( msr_index[idx] == reg )
{
+ uint64_t old = c->msr[idx];
+
c->msr[idx] = val;
+
+ if ( !sanitize_state(ctxt) )
+ {
+ c->msr[idx] = old;
+ break;
+ }
+
return X86EMUL_OKAY;
}
}
@@ -808,6 +839,30 @@ static void sanitize_input(struct x86_em
}
}
+/*
+ * Call this function from hooks potentially altering machine state into
+ * something that's not architecturally valid, yet which - as per above -
+ * the emulator relies on.
+ */
+static bool sanitize_state(struct x86_emulate_ctxt *ctxt)
+{
+ const struct fuzz_state *s = ctxt->data;
+ const struct fuzz_corpus *c = s->corpus;
+ const struct cpu_user_regs *regs = &c->regs;
+
+ if ( long_mode_active(ctxt) && !(c->cr[0] & X86_CR0_PG) )
+ return false;
+
+ if ( (c->cr[0] & X86_CR0_PG) && !(c->cr[0] & X86_CR0_PE) )
+ return false;
+
+ if ( (regs->rflags & X86_EFLAGS_VM) &&
+ (c->segments[x86_seg_cs].db || c->segments[x86_seg_ss].db) )
+ return false;
+
+ return true;
+}
+
int LLVMFuzzerInitialize(int *argc, char ***argv)
{
if ( !emul_test_init() )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] x86emul/fuzz: add a state sanitization function
2019-04-01 7:46 ` [PATCH v2] " Jan Beulich
@ 2019-04-01 10:44 ` George Dunlap
2019-04-01 12:05 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: George Dunlap @ 2019-04-01 10:44 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monne, Wei Liu, George Dunlap, Andrew Cooper
> On Apr 1, 2019, at 8:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
> This is to accompany sanitize_input(). Just like for initial state we
> want to have state between two emulated insns sane, at least as far as
> assumptions in the main emulator go. Do minimal checking after segment
> register, CR, and MSR writes, and roll back to the old value in case of
> failure (raising #GP(0) at the same time).
>
> In the particular case observed, a CR0 write clearing CR0.PE was
> followed by a VEX-encoded insn, which the decoder accepts based on
> guest address size, restricting things just outside of the 64-bit case
> (real and virtual modes don't allow VEX-encoded insns). Subsequently
> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Correct placement of new declaration in fuzz_write_segment().
>
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -76,6 +76,8 @@ static inline bool input_read(struct fuz
> return true;
> }
>
> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt);
> +
> static const char* const x86emul_return_string[] = {
> [X86EMUL_OKAY] = "X86EMUL_OKAY",
> [X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
> @@ -424,8 +426,19 @@ static int fuzz_write_segment(
> rc = maybe_fail(ctxt, "write_segment", true);
>
> if ( rc == X86EMUL_OKAY )
> + {
> + struct segment_register old = c->segments[seg];
> +
> c->segments[seg] = *reg;
>
> + if ( !sanitize_state(ctxt) )
> + {
> + c->segments[seg] = old;
> + x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
> + rc = X86EMUL_EXCEPTION;
> + }
> + }
> +
> return rc;
> }
>
> @@ -452,6 +465,7 @@ static int fuzz_write_cr(
> {
> struct fuzz_state *s = ctxt->data;
> struct fuzz_corpus *c = s->corpus;
> + unsigned long old;
> int rc;
>
> if ( reg >= ARRAY_SIZE(c->cr) )
> @@ -461,9 +475,17 @@ static int fuzz_write_cr(
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> + old = c->cr[reg];
> c->cr[reg] = val;
>
> - return X86EMUL_OKAY;
> + if ( !sanitize_state(ctxt) )
> + {
> + c->cr[reg] = old;
> + x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
> + rc = X86EMUL_EXCEPTION;
> + }
> +
> + return rc;
> }
>
> #define fuzz_read_xcr emul_test_read_xcr
> @@ -561,7 +583,16 @@ static int fuzz_write_msr(
> {
> if ( msr_index[idx] == reg )
> {
> + uint64_t old = c->msr[idx];
> +
> c->msr[idx] = val;
> +
> + if ( !sanitize_state(ctxt) )
> + {
> + c->msr[idx] = old;
> + break;
> + }
> +
> return X86EMUL_OKAY;
> }
> }
> @@ -808,6 +839,30 @@ static void sanitize_input(struct x86_em
> }
> }
>
> +/*
> + * Call this function from hooks potentially altering machine state into
> + * something that's not architecturally valid, yet which - as per above -
> + * the emulator relies on.
> + */
> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt)
> +{
> + const struct fuzz_state *s = ctxt->data;
> + const struct fuzz_corpus *c = s->corpus;
> + const struct cpu_user_regs *regs = &c->regs;
> +
> + if ( long_mode_active(ctxt) && !(c->cr[0] & X86_CR0_PG) )
> + return false;
> +
> + if ( (c->cr[0] & X86_CR0_PG) && !(c->cr[0] & X86_CR0_PE) )
> + return false;
> +
> + if ( (regs->rflags & X86_EFLAGS_VM) &&
> + (c->segments[x86_seg_cs].db || c->segments[x86_seg_ss].db) )
> + return false;
> +
> + return true;
> +}
Sorry, I didn’t read this function very well on Friday. It’s not actually doing any sanitation; rather, it’s checking whether the state is architecturally valid. Or more precisely: it’s checking whether the emulator's assumptions about the state still hold.
check_state? sanity_check_state?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] x86emul/fuzz: add a state sanitization function
2019-04-01 10:44 ` George Dunlap
@ 2019-04-01 12:05 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2019-04-01 12:05 UTC (permalink / raw)
To: george.dunlap; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 01.04.19 at 12:44, <George.Dunlap@citrix.com> wrote:
>> On Apr 1, 2019, at 8:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> +/*
>> + * Call this function from hooks potentially altering machine state into
>> + * something that's not architecturally valid, yet which - as per above -
>> + * the emulator relies on.
>> + */
>> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt)
>> +{
>> + const struct fuzz_state *s = ctxt->data;
>> + const struct fuzz_corpus *c = s->corpus;
>> + const struct cpu_user_regs *regs = &c->regs;
>> +
>> + if ( long_mode_active(ctxt) && !(c->cr[0] & X86_CR0_PG) )
>> + return false;
>> +
>> + if ( (c->cr[0] & X86_CR0_PG) && !(c->cr[0] & X86_CR0_PE) )
>> + return false;
>> +
>> + if ( (regs->rflags & X86_EFLAGS_VM) &&
>> + (c->segments[x86_seg_cs].db || c->segments[x86_seg_ss].db) )
>> + return false;
>> +
>> + return true;
>> +}
>
> Sorry, I didn’t read this function very well on Friday. It’s not actually
> doing any sanitation; rather, it’s checking whether the state is
> architecturally valid. Or more precisely: it’s checking whether the
> emulator's assumptions about the state still hold.
>
> check_state? sanity_check_state?
Hmm, yes - initially I was meaning to alter state, and then I decided
differently but didn't change the name. I'll go with check_state().
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] x86emul/fuzz: add a state sanity checking function
2019-03-29 14:54 [PATCH] x86emul/fuzz: add a state sanitization function Jan Beulich
2019-03-29 15:10 ` George Dunlap
2019-04-01 7:46 ` [PATCH v2] " Jan Beulich
@ 2019-04-02 13:01 ` Jan Beulich
2019-05-27 10:51 ` [Xen-devel] " George Dunlap
[not found] ` <5CA35D1302000000001041AA@prv1-mh.provo.novell.com>
3 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-04-02 13:01 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
This is to accompany sanitize_input(). Just like for initial state we
want to have state between two emulated insns sane, at least as far as
assumptions in the main emulator go. Do minimal checking after segment
register, CR, and MSR writes, and roll back to the old value in case of
failure (raising #GP(0) at the same time).
In the particular case observed, a CR0 write clearing CR0.PE was
followed by a VEX-encoded insn, which the decoder accepts based on
guest address size, restricting things just outside of the 64-bit case
(real and virtual modes don't allow VEX-encoded insns). Subsequently
_get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
clear) when trying to invoke YMM, ZMM, or OPMASK state.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Rename helper function to check_state().
v2: Correct placement of new declaration in fuzz_write_segment().
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -76,6 +76,8 @@ static inline bool input_read(struct fuz
return true;
}
+static bool check_state(struct x86_emulate_ctxt *ctxt);
+
static const char* const x86emul_return_string[] = {
[X86EMUL_OKAY] = "X86EMUL_OKAY",
[X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
@@ -424,8 +426,19 @@ static int fuzz_write_segment(
rc = maybe_fail(ctxt, "write_segment", true);
if ( rc == X86EMUL_OKAY )
+ {
+ struct segment_register old = c->segments[seg];
+
c->segments[seg] = *reg;
+ if ( !check_state(ctxt) )
+ {
+ c->segments[seg] = old;
+ x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
+ rc = X86EMUL_EXCEPTION;
+ }
+ }
+
return rc;
}
@@ -452,6 +465,7 @@ static int fuzz_write_cr(
{
struct fuzz_state *s = ctxt->data;
struct fuzz_corpus *c = s->corpus;
+ unsigned long old;
int rc;
if ( reg >= ARRAY_SIZE(c->cr) )
@@ -461,9 +475,17 @@ static int fuzz_write_cr(
if ( rc != X86EMUL_OKAY )
return rc;
+ old = c->cr[reg];
c->cr[reg] = val;
- return X86EMUL_OKAY;
+ if ( !check_state(ctxt) )
+ {
+ c->cr[reg] = old;
+ x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
+ rc = X86EMUL_EXCEPTION;
+ }
+
+ return rc;
}
#define fuzz_read_xcr emul_test_read_xcr
@@ -561,7 +583,16 @@ static int fuzz_write_msr(
{
if ( msr_index[idx] == reg )
{
+ uint64_t old = c->msr[idx];
+
c->msr[idx] = val;
+
+ if ( !check_state(ctxt) )
+ {
+ c->msr[idx] = old;
+ break;
+ }
+
return X86EMUL_OKAY;
}
}
@@ -811,6 +842,30 @@ static void sanitize_input(struct x86_em
}
}
+/*
+ * Call this function from hooks potentially altering machine state into
+ * something that's not architecturally valid, yet which - as per above -
+ * the emulator relies on.
+ */
+static bool check_state(struct x86_emulate_ctxt *ctxt)
+{
+ const struct fuzz_state *s = ctxt->data;
+ const struct fuzz_corpus *c = s->corpus;
+ const struct cpu_user_regs *regs = &c->regs;
+
+ if ( long_mode_active(ctxt) && !(c->cr[0] & X86_CR0_PG) )
+ return false;
+
+ if ( (c->cr[0] & X86_CR0_PG) && !(c->cr[0] & X86_CR0_PE) )
+ return false;
+
+ if ( (regs->rflags & X86_EFLAGS_VM) &&
+ (c->segments[x86_seg_cs].db || c->segments[x86_seg_ss].db) )
+ return false;
+
+ return true;
+}
+
int LLVMFuzzerInitialize(int *argc, char ***argv)
{
if ( !emul_test_init() )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3] x86emul/fuzz: add a state sanity checking function
@ 2019-05-27 10:51 ` George Dunlap
0 siblings, 0 replies; 13+ messages in thread
From: George Dunlap @ 2019-05-27 10:51 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
On 4/2/19 2:01 PM, Jan Beulich wrote:
> This is to accompany sanitize_input(). Just like for initial state we
> want to have state between two emulated insns sane, at least as far as
> assumptions in the main emulator go. Do minimal checking after segment
> register, CR, and MSR writes, and roll back to the old value in case of
> failure (raising #GP(0) at the same time).
>
> In the particular case observed, a CR0 write clearing CR0.PE was
> followed by a VEX-encoded insn, which the decoder accepts based on
> guest address size, restricting things just outside of the 64-bit case
> (real and virtual modes don't allow VEX-encoded insns). Subsequently
> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
That said, I wonder if there's a way to avoid the duplication between
sanitize_input() and check_state(). Another option would be to rework
sanitize_input() (perhaps as sanizite_state()):
* Accept a parameter saying whether to do optional changes (like
CANONICALIZE_MAYBE)
* Return a boolean saying whether any state was in fact sanitized.
Then the current callers of check_state() could instead call
sanitize_state(), and throw an exception if it returns 1. (Or some
variation thereof.)
Just a thought; I'm OK with checking this in as it is.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH v3] x86emul/fuzz: add a state sanity checking function
@ 2019-05-27 10:51 ` George Dunlap
0 siblings, 0 replies; 13+ messages in thread
From: George Dunlap @ 2019-05-27 10:51 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
On 4/2/19 2:01 PM, Jan Beulich wrote:
> This is to accompany sanitize_input(). Just like for initial state we
> want to have state between two emulated insns sane, at least as far as
> assumptions in the main emulator go. Do minimal checking after segment
> register, CR, and MSR writes, and roll back to the old value in case of
> failure (raising #GP(0) at the same time).
>
> In the particular case observed, a CR0 write clearing CR0.PE was
> followed by a VEX-encoded insn, which the decoder accepts based on
> guest address size, restricting things just outside of the 64-bit case
> (real and virtual modes don't allow VEX-encoded insns). Subsequently
> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
That said, I wonder if there's a way to avoid the duplication between
sanitize_input() and check_state(). Another option would be to rework
sanitize_input() (perhaps as sanizite_state()):
* Accept a parameter saying whether to do optional changes (like
CANONICALIZE_MAYBE)
* Return a boolean saying whether any state was in fact sanitized.
Then the current callers of check_state() could instead call
sanitize_state(), and throw an exception if it returns 1. (Or some
variation thereof.)
Just a thought; I'm OK with checking this in as it is.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add a state sanity checking function
@ 2019-05-27 11:41 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2019-05-27 11:41 UTC (permalink / raw)
To: george.dunlap
Cc: George Dunlap, Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 27.05.19 at 12:51, <george.dunlap@citrix.com> wrote:
> On 4/2/19 2:01 PM, Jan Beulich wrote:
>> This is to accompany sanitize_input(). Just like for initial state we
>> want to have state between two emulated insns sane, at least as far as
>> assumptions in the main emulator go. Do minimal checking after segment
>> register, CR, and MSR writes, and roll back to the old value in case of
>> failure (raising #GP(0) at the same time).
>>
>> In the particular case observed, a CR0 write clearing CR0.PE was
>> followed by a VEX-encoded insn, which the decoder accepts based on
>> guest address size, restricting things just outside of the 64-bit case
>> (real and virtual modes don't allow VEX-encoded insns). Subsequently
>> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
>> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Thanks.
> That said, I wonder if there's a way to avoid the duplication between
> sanitize_input() and check_state(). Another option would be to rework
> sanitize_input() (perhaps as sanizite_state()):
> * Accept a parameter saying whether to do optional changes (like
> CANONICALIZE_MAYBE)
> * Return a boolean saying whether any state was in fact sanitized.
>
> Then the current callers of check_state() could instead call
> sanitize_state(), and throw an exception if it returns 1. (Or some
> variation thereof.)
I did consider this at the time, but the two functions aren't doing
exactly the same validation. For example this
/* EFLAGS.VM not available in long mode */
if ( long_mode_active(ctxt) )
regs->rflags &= ~X86_EFLAGS_VM;
has no equivalent in check_state(), for it being an emulator bug
to ever set EFLAGS.VM in long mode. I therefore thought it would
be better to keep them separate despite there being partial
redundancy. If the set of checks grows, we could consider
factoring out the common subset into a helper function.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH v3] x86emul/fuzz: add a state sanity checking function
@ 2019-05-27 11:41 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2019-05-27 11:41 UTC (permalink / raw)
To: george.dunlap
Cc: George Dunlap, Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 27.05.19 at 12:51, <george.dunlap@citrix.com> wrote:
> On 4/2/19 2:01 PM, Jan Beulich wrote:
>> This is to accompany sanitize_input(). Just like for initial state we
>> want to have state between two emulated insns sane, at least as far as
>> assumptions in the main emulator go. Do minimal checking after segment
>> register, CR, and MSR writes, and roll back to the old value in case of
>> failure (raising #GP(0) at the same time).
>>
>> In the particular case observed, a CR0 write clearing CR0.PE was
>> followed by a VEX-encoded insn, which the decoder accepts based on
>> guest address size, restricting things just outside of the 64-bit case
>> (real and virtual modes don't allow VEX-encoded insns). Subsequently
>> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
>> clear) when trying to invoke YMM, ZMM, or OPMASK state.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Thanks.
> That said, I wonder if there's a way to avoid the duplication between
> sanitize_input() and check_state(). Another option would be to rework
> sanitize_input() (perhaps as sanizite_state()):
> * Accept a parameter saying whether to do optional changes (like
> CANONICALIZE_MAYBE)
> * Return a boolean saying whether any state was in fact sanitized.
>
> Then the current callers of check_state() could instead call
> sanitize_state(), and throw an exception if it returns 1. (Or some
> variation thereof.)
I did consider this at the time, but the two functions aren't doing
exactly the same validation. For example this
/* EFLAGS.VM not available in long mode */
if ( long_mode_active(ctxt) )
regs->rflags &= ~X86_EFLAGS_VM;
has no equivalent in check_state(), for it being an emulator bug
to ever set EFLAGS.VM in long mode. I therefore thought it would
be better to keep them separate despite there being partial
redundancy. If the set of checks grows, we could consider
factoring out the common subset into a helper function.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <5CA35D1302000000001041AA@prv1-mh.provo.novell.com>]