Re: [PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access, write}() omissions
On 29.05.2020 17:03, Andrew Cooper wrote:
> On 29/05/2020 14:29, Jan Beulich wrote:
>> On 29.05.2020 14:18, Andrew Cooper wrote:
>>> On 25/05/2020 15:26, Jan Beulich wrote:
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
return state->ea.mem.off;
}
+/*
+ * This function means to return 'true' for all supported insns with
explicit
+ * accesses to memory. This means also insns which don't have an explicit
+ * memory operand (like POP), but it does not mean e.g. segment selector
+ * loads, where the descriptor table access is considered an implicit one.
+ */
bool
x86_insn_is_mem_access(const struct x86_emulate_state *state,
const struct x86_emulate_ctxt *ctxt)
{
+if ( mode_64bit() && state->not_64bit )
+return false;
>>> Is this path actually used?
>> Yes, it is. It's only x86_emulate() which has
>>
>> generate_exception_if(state->not_64bit && mode_64bit(), EXC_UD);
>>
>> right now.
>
> Oh. That is a bit awkward.
>
>>> state->not_64bit ought to fail instruction
>>> decode, at which point we wouldn't have a valid state to be used here.
>> x86_decode() currently doesn't have much raising of #UD at all, I
>> think. If it wasn't like this, the not_64bit field wouldn't be
>> needed - it's used only to communicate from decode to execute.
>> We're not entirely consistent with this though, seeing in
>> x86_decode_onebyte(), a few lines below the block of case labels
>> setting that field,
>>
>> case 0x9a: /* call (far, absolute) */
>> case 0xea: /* jmp (far, absolute) */
>> generate_exception_if(mode_64bit(), EXC_UD);
>
> This is because there is no legitimate way to determine the end of the
> instruction.
>
> Most of the not_64bit instructions do have a well-defined end, even if
> they aren't permitted for use.
I wouldn't bet on that. Just look at the re-use of opcode D6
(salc in 32-bit) by L1OM for an extreme example. There's nothing
we can say on how any of the reserved opcodes may get re-used.
Prior to AVX / AVX512 we'd have estimated C4, C5, and 62 wrongly
as well (but that's true independent of execution mode).
>> We could certainly drop the field and raise #UD during decode
>> already, but don't you think we then should do so for all
>> encodings that ultimately lead to #UD, e.g. also for AVX insns
>> without AVX available to the guest? This would make
>> x86_decode() quite a bit larger, as it would then also need to
>> have a giant switch() (or something else that's suitable to
>> cover all cases).
>
> I think there is a semantic difference between "we can't parse the
> instruction", and "we can parse it, but it's not legitimate in this
> context".
>
> I don't think our exact split is correct, but I don't think moving all
> #UD checking into x86_decode() is correct either.
Do you have any clear, sufficiently simple rule in mind for where
we could draw the boundary?
Jan
Re: [PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access, write}() omissions
On 29/05/2020 14:29, Jan Beulich wrote:
> On 29.05.2020 14:18, Andrew Cooper wrote:
>> On 25/05/2020 15:26, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
>>> return state->ea.mem.off;
>>> }
>>>
>>> +/*
>>> + * This function means to return 'true' for all supported insns with
>>> explicit
>>> + * accesses to memory. This means also insns which don't have an explicit
>>> + * memory operand (like POP), but it does not mean e.g. segment selector
>>> + * loads, where the descriptor table access is considered an implicit one.
>>> + */
>>> bool
>>> x86_insn_is_mem_access(const struct x86_emulate_state *state,
>>> const struct x86_emulate_ctxt *ctxt)
>>> {
>>> +if ( mode_64bit() && state->not_64bit )
>>> +return false;
>> Is this path actually used?
> Yes, it is. It's only x86_emulate() which has
>
> generate_exception_if(state->not_64bit && mode_64bit(), EXC_UD);
>
> right now.
Oh. That is a bit awkward.
>> state->not_64bit ought to fail instruction
>> decode, at which point we wouldn't have a valid state to be used here.
> x86_decode() currently doesn't have much raising of #UD at all, I
> think. If it wasn't like this, the not_64bit field wouldn't be
> needed - it's used only to communicate from decode to execute.
> We're not entirely consistent with this though, seeing in
> x86_decode_onebyte(), a few lines below the block of case labels
> setting that field,
>
> case 0x9a: /* call (far, absolute) */
> case 0xea: /* jmp (far, absolute) */
> generate_exception_if(mode_64bit(), EXC_UD);
This is because there is no legitimate way to determine the end of the
instruction.
Most of the not_64bit instructions do have a well-defined end, even if
they aren't permitted for use.
> We could certainly drop the field and raise #UD during decode
> already, but don't you think we then should do so for all
> encodings that ultimately lead to #UD, e.g. also for AVX insns
> without AVX available to the guest? This would make
> x86_decode() quite a bit larger, as it would then also need to
> have a giant switch() (or something else that's suitable to
> cover all cases).
I think there is a semantic difference between "we can't parse the
instruction", and "we can parse it, but it's not legitimate in this
context".
I don't think our exact split is correct, but I don't think moving all
#UD checking into x86_decode() is correct either.
~Andrew
Re: [PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access, write}() omissions
On 29.05.2020 14:18, Andrew Cooper wrote:
> On 25/05/2020 15:26, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
>> return state->ea.mem.off;
>> }
>>
>> +/*
>> + * This function means to return 'true' for all supported insns with
>> explicit
>> + * accesses to memory. This means also insns which don't have an explicit
>> + * memory operand (like POP), but it does not mean e.g. segment selector
>> + * loads, where the descriptor table access is considered an implicit one.
>> + */
>> bool
>> x86_insn_is_mem_access(const struct x86_emulate_state *state,
>> const struct x86_emulate_ctxt *ctxt)
>> {
>> +if ( mode_64bit() && state->not_64bit )
>> +return false;
>
> Is this path actually used?
Yes, it is. It's only x86_emulate() which has
generate_exception_if(state->not_64bit && mode_64bit(), EXC_UD);
right now.
> state->not_64bit ought to fail instruction
> decode, at which point we wouldn't have a valid state to be used here.
x86_decode() currently doesn't have much raising of #UD at all, I
think. If it wasn't like this, the not_64bit field wouldn't be
needed - it's used only to communicate from decode to execute.
We're not entirely consistent with this though, seeing in
x86_decode_onebyte(), a few lines below the block of case labels
setting that field,
case 0x9a: /* call (far, absolute) */
case 0xea: /* jmp (far, absolute) */
generate_exception_if(mode_64bit(), EXC_UD);
We could certainly drop the field and raise #UD during decode
already, but don't you think we then should do so for all
encodings that ultimately lead to #UD, e.g. also for AVX insns
without AVX available to the guest? This would make
x86_decode() quite a bit larger, as it would then also need to
have a giant switch() (or something else that's suitable to
cover all cases).
> Everything else looks ok, so Acked-by: Andrew Cooper
>
Thanks.
Jan
Re: [PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access, write}() omissions
On 25/05/2020 15:26, Jan Beulich wrote:
> First of all explain in comments what the functions' purposes are. Then
> make them actually match their comments.
>
> Note that fc6fa977be54 ("x86emul: extend x86_insn_is_mem_write()
> coverage") didn't actually fix the function's behavior for {,V}STMXCSR:
> Both are covered by generic code higher up in the function, due to
> x86_decode_twobyte() already doing suitable adjustments. And VSTMXCSR
> wouldn't have been covered anyway without a further X86EMUL_OPC_VEX()
> case label. Keep the inner case label in a comment for reference.
>
> Signed-off-by: Jan Beulich
> ---
> v10: Move ARPL case to earlier switch() x86_insn_is_mem_write(). Make
> group 5 handling actually work there. Drop VMPTRST case. Also
> handle CLFLUSH*, CLWB, UDn, and remaining PREFETCH* in
> x86_insn_is_mem_access().
> v9: New.
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
> return state->ea.mem.off;
> }
>
> +/*
> + * This function means to return 'true' for all supported insns with explicit
> + * accesses to memory. This means also insns which don't have an explicit
> + * memory operand (like POP), but it does not mean e.g. segment selector
> + * loads, where the descriptor table access is considered an implicit one.
> + */
> bool
> x86_insn_is_mem_access(const struct x86_emulate_state *state,
> const struct x86_emulate_ctxt *ctxt)
> {
> +if ( mode_64bit() && state->not_64bit )
> +return false;
Is this path actually used? state->not_64bit ought to fail instruction
decode, at which point we wouldn't have a valid state to be used here.
Everything else looks ok, so Acked-by: Andrew Cooper
[PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access,write}() omissions
First of all explain in comments what the functions' purposes are. Then
make them actually match their comments.
Note that fc6fa977be54 ("x86emul: extend x86_insn_is_mem_write()
coverage") didn't actually fix the function's behavior for {,V}STMXCSR:
Both are covered by generic code higher up in the function, due to
x86_decode_twobyte() already doing suitable adjustments. And VSTMXCSR
wouldn't have been covered anyway without a further X86EMUL_OPC_VEX()
case label. Keep the inner case label in a comment for reference.
Signed-off-by: Jan Beulich
---
v10: Move ARPL case to earlier switch() x86_insn_is_mem_write(). Make
group 5 handling actually work there. Drop VMPTRST case. Also
handle CLFLUSH*, CLWB, UDn, and remaining PREFETCH* in
x86_insn_is_mem_access().
v9: New.
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
return state->ea.mem.off;
}
+/*
+ * This function means to return 'true' for all supported insns with explicit
+ * accesses to memory. This means also insns which don't have an explicit
+ * memory operand (like POP), but it does not mean e.g. segment selector
+ * loads, where the descriptor table access is considered an implicit one.
+ */
bool
x86_insn_is_mem_access(const struct x86_emulate_state *state,
const struct x86_emulate_ctxt *ctxt)
{
+if ( mode_64bit() && state->not_64bit )
+return false;
+
if ( state->ea.type == OP_MEM )
-return ctxt->opcode != 0x8d /* LEA */ &&
- (ctxt->opcode != X86EMUL_OPC(0x0f, 0x01) ||
-(state->modrm_reg & 7) != 7) /* INVLPG */;
+{
+switch ( ctxt->opcode )
+{
+case 0x8d: /* LEA */
+case X86EMUL_OPC(0x0f, 0x0d): /* PREFETCH */
+case X86EMUL_OPC(0x0f, 0x18)
+ ... X86EMUL_OPC(0x0f, 0x1f): /* NOP space */
+case X86EMUL_OPC_66(0x0f, 0x18)
+ ... X86EMUL_OPC_66(0x0f, 0x1f): /* NOP space */
+case X86EMUL_OPC_F3(0x0f, 0x18)
+ ... X86EMUL_OPC_F3(0x0f, 0x1f): /* NOP space */
+case X86EMUL_OPC_F2(0x0f, 0x18)
+ ... X86EMUL_OPC_F2(0x0f, 0x1f): /* NOP space */
+case X86EMUL_OPC(0x0f, 0xb9): /* UD1 */
+case X86EMUL_OPC(0x0f, 0xff): /* UD0 */
+return false;
+
+case X86EMUL_OPC(0x0f, 0x01):
+return (state->modrm_reg & 7) != 7; /* INVLPG */
+
+case X86EMUL_OPC(0x0f, 0xae):
+return (state->modrm_reg & 7) != 7; /* CLFLUSH */
+
+case X86EMUL_OPC_66(0x0f, 0xae):
+return (state->modrm_reg & 7) < 6; /* CLWB, CLFLUSHOPT */
+}
+
+return true;
+}
switch ( ctxt->opcode )
{
+case 0x06 ... 0x07: /* PUSH / POP %es */
+case 0x0e: /* PUSH %cs */
+case 0x16 ... 0x17: /* PUSH / POP %ss */
+case 0x1e ... 0x1f: /* PUSH / POP %ds */
+case 0x50 ... 0x5f: /* PUSH / POP reg */
+case 0x60 ... 0x61: /* PUSHA / POPA */
+case 0x68: case 0x6a: /* PUSH imm */
case 0x6c ... 0x6f: /* INS / OUTS */
+case 0x8f: /* POP r/m */
+case 0x9a: /* CALL (far, direct) */
+case 0x9c ... 0x9d: /* PUSHF / POPF */
case 0xa4 ... 0xa7: /* MOVS / CMPS */
case 0xaa ... 0xaf: /* STOS / LODS / SCAS */
+case 0xc2 ... 0xc3: /* RET (near) */
+case 0xc8 ... 0xc9: /* ENTER / LEAVE */
+case 0xca ... 0xcb: /* RET (far) */
case 0xd7: /* XLAT */
+case 0xe8: /* CALL (near, direct) */
+case X86EMUL_OPC(0x0f, 0xa0): /* PUSH %fs */
+case X86EMUL_OPC(0x0f, 0xa1): /* POP %fs */
+case X86EMUL_OPC(0x0f, 0xa8): /* PUSH %gs */
+case X86EMUL_OPC(0x0f, 0xa9): /* POP %gs */
CASE_SIMD_PACKED_INT_VEX(0x0f, 0xf7): /* MASKMOV{Q,DQU} */
/* VMASKMOVDQU */
return true;
+case 0xff:
+switch ( state->modrm_reg & 7 )
+{
+case 2: /* CALL (near, indirect) */
+case 6: /* PUSH r/m */
+return true;
+}
+break;
+
case X86EMUL_OPC(0x0f, 0x01):
/* Cover CLZERO. */
return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
@@ -11501,10 +11563,20 @@ x86_insn_is_mem_access(const struct x86_
return false;
}
+/*
+ * This function means to return 'true' for all supported insns with explicit
+ * writes to memory. This means also insns which don't have an explicit
+ * memory operand (like PUSH), but it does not mean e.g. segment selector
+ * loads, where the (possible) descriptor table write is considered an
+ * implicit access.
+ */
bool
x86_insn_is_mem_write(const struct x86_emulate_state *state,
const struct x86_emulate_ctxt *ctxt)
{
+if ( mode_64bit() && state->not_64bit )
+return false;
+
switch ( state->desc & DstMask )
{
case DstMem:
@@ -11516,19 +11588,48 @@
