| Message ID | 39de64e1-c615-4ec3-ad05-ff99f27a8e30@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86emul: BMI{1,2} corrections | expand |
On 12/11/2024 3:00 pm, Jan Beulich wrote: > While result values and other status flags are unaffected as long as we > can ignore the case of registers having their upper 32 bits non-zero > outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we > mistakenly re-execute the original insn with VEX.W set. > > Note that the memory access, if any, is correctly carried out as 32-bit > regardless of VEX.W. I don't understand why this is true. If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit mode, it will have an operand size of 64. I can believe that ... > Internal state also isn't leaked, as the field the > memory value is read into (which is then wrongly accessed as a 64-bit > quantity when executing the stub) is pre-initialized to zero. ... everything else treats the memory operand as 32bit, and uses the bottom half of the internal buffer, and generally does the right thing. ~Andrew
On 13.11.2024 01:24, Andrew Cooper wrote: > On 12/11/2024 3:00 pm, Jan Beulich wrote: >> While result values and other status flags are unaffected as long as we >> can ignore the case of registers having their upper 32 bits non-zero >> outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we >> mistakenly re-execute the original insn with VEX.W set. >> >> Note that the memory access, if any, is correctly carried out as 32-bit >> regardless of VEX.W. > > I don't understand why this is true. This talks about the access to guest memory, which is op_bytes based. And op_bytes determination handles VEX.W correctly afaics. I've added "guest" near the start of the sentence for clarification. > If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit > mode, it will have an operand size of 64. > > I can believe that ... > >> Internal state also isn't leaked, as the field the >> memory value is read into (which is then wrongly accessed as a 64-bit >> quantity when executing the stub) is pre-initialized to zero. > > ... everything else treats the memory operand as 32bit, and uses the > bottom half of the internal buffer, and generally does the right thing. No, if I'm getting right what you say it was the other way around: Right now we - read guest memory (ahead of the big switch()); that's always a 32-bit access for VEX-encoded GPR insns in 32-bit code (the value is read into an internal field which is pre-set to zero, i.e. when used as a 64-bit quantity, it's effectively the zero-extended value that was read from guest memory), - emit a VEX-encoded insn into the stub with VEX.W set, - execute that insn, resulting in a 64-bit memory access to the internal field, where as per above the upper half is zero. It's only this way that it can be explained why the new testcase added would previously have failed (wrongly set EFLAGS.SF). Jan
On 13/11/2024 8:01 am, Jan Beulich wrote: > On 13.11.2024 01:24, Andrew Cooper wrote: >> On 12/11/2024 3:00 pm, Jan Beulich wrote: >>> While result values and other status flags are unaffected as long as we >>> can ignore the case of registers having their upper 32 bits non-zero >>> outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we >>> mistakenly re-execute the original insn with VEX.W set. >>> >>> Note that the memory access, if any, is correctly carried out as 32-bit >>> regardless of VEX.W. >> I don't understand why this is true. > This talks about the access to guest memory, which is op_bytes based. > And op_bytes determination handles VEX.W correctly afaics. I've added > "guest" near the start of the sentence for clarification. Ah - that makes things much clearer. I had neglected to consider the access to guest memory. In addition to a "guest" earlier, I'd suggest having a new paragraph at this point, and ... > >> If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit >> mode, it will have an operand size of 64. >> >> I can believe that ... >> >>> Internal state also isn't leaked, as the field the >>> memory value is read into (which is then wrongly accessed as a 64-bit >>> quantity when executing the stub) is pre-initialized to zero. ... this reading: "The emulator-local memory operand will be accessed as a 64-bit quantity, but it is pre-initialised to zero so no internal state an leak" or similar. ~Andrew
On 13/11/2024 1:31 pm, Andrew Cooper wrote: > On 13/11/2024 8:01 am, Jan Beulich wrote: >> On 13.11.2024 01:24, Andrew Cooper wrote: >>> On 12/11/2024 3:00 pm, Jan Beulich wrote: >>>> While result values and other status flags are unaffected as long as we >>>> can ignore the case of registers having their upper 32 bits non-zero >>>> outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we >>>> mistakenly re-execute the original insn with VEX.W set. >>>> >>>> Note that the memory access, if any, is correctly carried out as 32-bit >>>> regardless of VEX.W. >>> I don't understand why this is true. >> This talks about the access to guest memory, which is op_bytes based. >> And op_bytes determination handles VEX.W correctly afaics. I've added >> "guest" near the start of the sentence for clarification. > Ah - that makes things much clearer. > > I had neglected to consider the access to guest memory. > > In addition to a "guest" earlier, I'd suggest having a new paragraph at > this point, and ... > >>> If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit >>> mode, it will have an operand size of 64. >>> >>> I can believe that ... >>> >>>> Internal state also isn't leaked, as the field the >>>> memory value is read into (which is then wrongly accessed as a 64-bit >>>> quantity when executing the stub) is pre-initialized to zero. > ... this reading: > > "The emulator-local memory operand will be accessed as a 64-bit > quantity, but it is pre-initialised to zero so no internal state an leak" > > or similar. Oh, and Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> ~Andrew
On 13.11.2024 14:32, Andrew Cooper wrote: > On 13/11/2024 1:31 pm, Andrew Cooper wrote: >> On 13/11/2024 8:01 am, Jan Beulich wrote: >>> On 13.11.2024 01:24, Andrew Cooper wrote: >>>> On 12/11/2024 3:00 pm, Jan Beulich wrote: >>>>> While result values and other status flags are unaffected as long as we >>>>> can ignore the case of registers having their upper 32 bits non-zero >>>>> outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we >>>>> mistakenly re-execute the original insn with VEX.W set. >>>>> >>>>> Note that the memory access, if any, is correctly carried out as 32-bit >>>>> regardless of VEX.W. >>>> I don't understand why this is true. >>> This talks about the access to guest memory, which is op_bytes based. >>> And op_bytes determination handles VEX.W correctly afaics. I've added >>> "guest" near the start of the sentence for clarification. >> Ah - that makes things much clearer. >> >> I had neglected to consider the access to guest memory. >> >> In addition to a "guest" earlier, I'd suggest having a new paragraph at >> this point, and ... >> >>>> If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit >>>> mode, it will have an operand size of 64. >>>> >>>> I can believe that ... >>>> >>>>> Internal state also isn't leaked, as the field the >>>>> memory value is read into (which is then wrongly accessed as a 64-bit >>>>> quantity when executing the stub) is pre-initialized to zero. >> ... this reading: >> >> "The emulator-local memory operand will be accessed as a 64-bit >> quantity, but it is pre-initialised to zero so no internal state an leak" >> >> or similar. That's to _replace_ the "Internal state ..." sentence then, rather than an added separate sentence / paragraph? It says exactly the same, after all. > Oh, and Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks, but I'll wait some for clarification above. Jan
On 14/11/2024 9:39 am, Jan Beulich wrote: > On 13.11.2024 14:32, Andrew Cooper wrote: >> On 13/11/2024 1:31 pm, Andrew Cooper wrote: >>> On 13/11/2024 8:01 am, Jan Beulich wrote: >>>> On 13.11.2024 01:24, Andrew Cooper wrote: >>>>> On 12/11/2024 3:00 pm, Jan Beulich wrote: >>>>>> While result values and other status flags are unaffected as long as we >>>>>> can ignore the case of registers having their upper 32 bits non-zero >>>>>> outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we >>>>>> mistakenly re-execute the original insn with VEX.W set. >>>>>> >>>>>> Note that the memory access, if any, is correctly carried out as 32-bit >>>>>> regardless of VEX.W. >>>>> I don't understand why this is true. >>>> This talks about the access to guest memory, which is op_bytes based. >>>> And op_bytes determination handles VEX.W correctly afaics. I've added >>>> "guest" near the start of the sentence for clarification. >>> Ah - that makes things much clearer. >>> >>> I had neglected to consider the access to guest memory. >>> >>> In addition to a "guest" earlier, I'd suggest having a new paragraph at >>> this point, and ... >>> >>>>> If we write out a VEX.W=1 form of BEXTR/etc and emulate while in 64bit >>>>> mode, it will have an operand size of 64. >>>>> >>>>> I can believe that ... >>>>> >>>>>> Internal state also isn't leaked, as the field the >>>>>> memory value is read into (which is then wrongly accessed as a 64-bit >>>>>> quantity when executing the stub) is pre-initialized to zero. >>> ... this reading: >>> >>> "The emulator-local memory operand will be accessed as a 64-bit >>> quantity, but it is pre-initialised to zero so no internal state an leak" >>> >>> or similar. > That's to _replace_ the "Internal state ..." sentence then, rather than an > added separate sentence / paragraph? It says exactly the same, after all. Ideally, yes. I think it's clearer this way around. ~Andrew
--- a/tools/tests/x86_emulator/test_x86_emulator.c +++ b/tools/tests/x86_emulator/test_x86_emulator.c @@ -2021,6 +2021,30 @@ int main(int argc, char **argv) (EFLAGS_ALWAYS_SET | X86_EFLAGS_SF) || !check_eip(blsr) ) goto fail; + +#ifdef __x86_64__ + /* Re-test with VEX.W set while emulating 32-bit mode. */ + ctxt.lma = 0; + ctxt.addr_size = 32; + ctxt.sp_size = 32; + + memcpy(instr, blsr, blsr_end - blsr); + instr[2] |= 0x80; + regs.rip = (unsigned long)&instr[0]; + regs.eflags = EFLAGS_ALWAYS_SET | X86_EFLAGS_OF | X86_EFLAGS_ZF | \ + X86_EFLAGS_CF; + rc = x86_emulate(&ctxt, &emulops); + if ( (rc != X86EMUL_OKAY) || regs.ecx != 0xfedcba90 || + (regs.eflags & (EFLAGS_MASK & ~(X86_EFLAGS_AF | X86_EFLAGS_PF))) != + (EFLAGS_ALWAYS_SET | X86_EFLAGS_SF) || + (regs.rip != (unsigned long)&instr[blsr_end - blsr]) ) + goto fail; + + ctxt.lma = 1; + ctxt.addr_size = 64; + ctxt.sp_size = 64; +#endif + printf("okay\n"); } else --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -6896,6 +6896,8 @@ x86_emulate( *pvex = vex; pvex->b = 1; pvex->r = 1; + if ( !mode_64bit() ) + pvex->w = 0; pvex->reg = 0xf; /* rAX */ buf[3] = b; buf[4] = 0x09; /* reg=rCX r/m=(%rCX) */ @@ -6930,6 +6932,8 @@ x86_emulate( *pvex = vex; pvex->b = 1; pvex->r = 1; + if ( !mode_64bit() ) + pvex->w = 0; pvex->reg = 0xf; /* rAX */ buf[3] = b; buf[4] = (modrm & 0x38) | 0x01; /* r/m=(%rCX) */
While result values and other status flags are unaffected as long as we can ignore the case of registers having their upper 32 bits non-zero outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we mistakenly re-execute the original insn with VEX.W set. Note that the memory access, if any, is correctly carried out as 32-bit regardless of VEX.W. Internal state also isn't leaked, as the field the memory value is read into (which is then wrongly accessed as a 64-bit quantity when executing the stub) is pre-initialized to zero. Fixes: 771daacd197a ("x86emul: support BMI1 insns") Signed-off-by: Jan Beulich <jbeulich@suse.com>