[PATCH] KVM: x86/emulator: Fix handing of POP SS to correctly set interruptibility

[PATCH] KVM: x86/emulator: Fix handing of POP SS to correctly set interruptibility

[Michal Luczaj]

The emulator checks the wrong variable while setting the CPU
interruptibility state.  Fix the condition.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
I'll follow up with a testcase.

 arch/x86/kvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b4eeb7c75dfa..5cfd07f483b3 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1967,7 +1967,7 @@ static int em_pop_sreg(struct x86_emulate_ctxt *ctxt)
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
-	if (ctxt->modrm_reg == VCPU_SREG_SS)
+	if (seg == VCPU_SREG_SS)
 		ctxt->interruptibility = KVM_X86_SHADOW_INT_MOV_SS;
 	if (ctxt->op_bytes > 2)
 		rsp_increment(ctxt, ctxt->op_bytes - 2);
-- 
2.37.2

[kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Michal Luczaj]

Verify that CPU interruptibility due to POP SS is set correctly (and #DB
is suppressed).

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Initially I wanted to test #DB suppression with EFLAGS.TF=1, but it
turned out that making the emulator set ctxt->interruptibility like this

	asm volatile("pushf\n\t"
		     "push %[flags]\n\t"
		     "popf\n\t"
		     KVM_FEP "mov %[ss], %%ss\n\t"
		     "popf"
		     :
		     : [ss] "r" (read_ss()),
		       [flags] "r" (read_rflags() | X86_EFLAGS_TF)
		     : "memory");

results in "KVM: entry failed, hardware error 0x80000021".
kvm_intel.dump_invalid_vmcs=1 tells me at that moment

Interruptibility = 00000002
DebugExceptions = 0x0000000000000000

so perhaps it's related to the problem described in
https://lkml.kernel.org/kvm/20220120000624.655815-1-seanjc@google.com/ ?
That said, I don't know if combining FEP+blocking+TF+DB is a misuse on
my side, a bug, or a detail that happens to be unimplemented.

Anyway, the POP-SS blocking test avoids touching EFLAGS.TF by using
DR0/7.

Note that doing this the ASM_TRY() way would require extending
setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
(one without the initial `movl $0, %%gs:4`).

 x86/Makefile.i386 |  3 ++-
 x86/popss.c       | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 x86/unittests.cfg |  4 ++++
 3 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 x86/popss.c

diff --git a/x86/Makefile.i386 b/x86/Makefile.i386
index 0a845e6..b570ae2 100644
--- a/x86/Makefile.i386
+++ b/x86/Makefile.i386
@@ -9,6 +9,7 @@ arch_LDFLAGS = -m elf_i386
 cflatobjs += lib/x86/setjmp32.o lib/ldiv32.o
 
 tests = $(TEST_DIR)/taskswitch.$(exe) $(TEST_DIR)/taskswitch2.$(exe) \
-	$(TEST_DIR)/cmpxchg8b.$(exe) $(TEST_DIR)/la57.$(exe)
+	$(TEST_DIR)/cmpxchg8b.$(exe) $(TEST_DIR)/la57.$(exe) \
+	$(TEST_DIR)/popss.$(exe)
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
diff --git a/x86/popss.c b/x86/popss.c
new file mode 100644
index 0000000..7201f1e
--- /dev/null
+++ b/x86/popss.c
@@ -0,0 +1,59 @@
+#include <asm/debugreg.h>
+
+#include "processor.h"
+#include "libcflat.h"
+#include "vmalloc.h"
+#include "desc.h"
+
+static void test_pop_ss_blocking_handler(struct ex_regs *regs)
+{
+	extern char test_pop_ss_blocking_cont;
+
+	regs->rip = (ulong)&test_pop_ss_blocking_cont;
+}
+
+static void test_pop_ss_blocking(void)
+{
+	extern char db_blocked;
+	int success = 0;
+	handler old;
+
+	old = handle_exception(DB_VECTOR, test_pop_ss_blocking_handler);
+
+	write_dr0(&db_blocked);
+	write_dr7(DR7_FIXED_1 |
+		  DR7_GLOBAL_ENABLE_DRx(0) |
+		  DR7_EXECUTE_DRx(0) |
+		  DR7_LEN_1_DRx(0));
+
+	/*
+	 * The idea is that #DB on the instruction following POP SS should be
+	 * suppressed. If the exception is actually suppressed, `success` gets
+	 * set to 1, otherwise exception handler advances RIP away.
+	 */
+	asm volatile("push %[ss]\n\t"
+		     KVM_FEP "pop %%ss\n\t"
+		     "db_blocked: mov $1, %[success]\n\t"
+		     "test_pop_ss_blocking_cont:"
+		     : [success] "+g" (success)
+		     : [ss] "r" (read_ss())
+		     : "memory");
+
+	write_dr7(DR7_FIXED_1);
+	handle_exception(DB_VECTOR, old);
+
+	report(success, "#DB suppressed after POP SS");
+}
+
+int main(void)
+{
+	setup_vm();
+
+	if (is_fep_available())
+		test_pop_ss_blocking();
+	else
+		report_skip("skipping POP-SS blocking test, "
+			    "use kvm.force_emulation_prefix=1 to enable");
+
+	return report_summary();
+}
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index ed65185..8d4e917 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -305,6 +305,10 @@ extra_params = -cpu qemu64,+umip
 file = la57.flat
 arch = i386
 
+[popss]
+file = popss.flat
+arch = i386
+
 [vmx]
 file = vmx.flat
 extra_params = -cpu max,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_test -atomic_switch_overflow_msrs_test -vmx_init_signal_test -vmx_apic_passthrough_tpr_threshold_test -apic_reg_virt_test -virt_x2apic_mode_test -vmx_pf_exception_test -vmx_pf_no_vpid_test -vmx_pf_invvpid_test -vmx_pf_vpid_test"
-- 
2.37.2

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Michal Luczaj]

On 8/22/22 00:06, Michal Luczaj wrote:
> Note that doing this the ASM_TRY() way would require extending
> setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
> (one without the initial `movl $0, %%gs:4`).

Replying to self as I was wrong regarding the need for another ASM_TRY() variant.
Once setup_idt() is told to handle #DB, test can be simplified to something like

	asm volatile("push %[ss]\n\t"
		     __ASM_TRY(KVM_FEP "pop %%ss", "1f")
		     "ex_blocked: mov $1, %[success]\n\t"
		     "1:"
		     : [success] "+g" (success)
		     : [ss] "r" (read_ss())
		     : "memory");

Should I resend the patch?

Sorry for the noise,
Michal

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Sean Christopherson]

On Mon, Aug 22, 2022, Michal Luczaj wrote:
> On 8/22/22 00:06, Michal Luczaj wrote:
> > Note that doing this the ASM_TRY() way would require extending
> > setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
> > (one without the initial `movl $0, %%gs:4`).
> 
> Replying to self as I was wrong regarding the need for another ASM_TRY() variant.
> Once setup_idt() is told to handle #DB,

Hmm, it might be a moot point for this patch (see below), but my vote is to have
setup_idt() wire up all known handlers to check_exception_table().  I don't see
any reason to skip some vectors.  Code with __ASM_TRY() will explode no matter what,
so it's not like it risks suppressing completely unexpected faults.

	for (i = 0; i < 32; i++) {
		if (!idt_handlers[i])
			continue;

		set_idt_entry(i, idt_handlers[i], 0);
		handle_exception(i, check_exception_table);
	}

> test can be simplified to something like
> 
> 	asm volatile("push %[ss]\n\t"
> 		     __ASM_TRY(KVM_FEP "pop %%ss", "1f")
> 		     "ex_blocked: mov $1, %[success]\n\t"

So I'm 99% certain this only passes because KVM doesn't emulate the `mov $1, %[success]`.
kvm_vcpu_check_code_breakpoint() honors EFLAGS.RF, but not MOV/POP_SS blocking.
I.e. I would expect this to fail if the `mov` were also tagged KVM_FEP.

Single-step and data #DBs appear to be broken as well, I don't see anything that
will suppress them for MOV/POP_SS.  Of course, KVM doesn't emulate data #DBs at
all, so testing that case is probably not worthwhile at this time.

The reason I say the setup_idt() change is a moot point is because I think it
would be better to put this testcase into debug.c (where "this" testcase is really
going to be multiple testcases).  With macro shenanigans (or code patching), it
should be fairly easy to run all testcases with both FEP=0 and FEP=1.

Then it's "just" a matter of adding a code #DB testcase.  __run_single_step_db_test()
and run_ss_db_test() aren't fundamentally tied to single-step, i.e. can simply be
renamed.  For simplicity, I'd say just skip the usermode test for code #DBs, IMO
it's not worth the extra coverage.

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Nadav Amit]

On Aug 22, 2022, at 8:42 AM, Sean Christopherson <seanjc@google.com> wrote:

> On Mon, Aug 22, 2022, Michal Luczaj wrote:
>> On 8/22/22 00:06, Michal Luczaj wrote:
>>> Note that doing this the ASM_TRY() way would require extending
>>> setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
>>> (one without the initial `movl $0, %%gs:4`).
>> 
>> Replying to self as I was wrong regarding the need for another ASM_TRY() variant.
>> Once setup_idt() is told to handle #DB,
> 
> Hmm, it might be a moot point for this patch (see below), but my vote is to have
> setup_idt() wire up all known handlers to check_exception_table().  I don't see
> any reason to skip some vectors.  Code with __ASM_TRY() will explode no matter what,
> so it's not like it risks suppressing completely unexpected faults.

I would just like to point out that this whole FEP approach is not friendly
for other hypervisors or bare-metal testings. While people might have mixed
feelings about other hypervisors, the benefit of running the tests on
bare-metal should be clear.

Specifically in this test, it is rather simple to avoid using FEP.

Oh, well.

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Sean Christopherson]

On Mon, Aug 22, 2022, Nadav Amit wrote:
> On Aug 22, 2022, at 8:42 AM, Sean Christopherson <seanjc@google.com> wrote:
> 
> > On Mon, Aug 22, 2022, Michal Luczaj wrote:
> >> On 8/22/22 00:06, Michal Luczaj wrote:
> >>> Note that doing this the ASM_TRY() way would require extending
> >>> setup_idt() (to handle #DB) and introducing another ASM_TRY() variant
> >>> (one without the initial `movl $0, %%gs:4`).
> >> 
> >> Replying to self as I was wrong regarding the need for another ASM_TRY() variant.
> >> Once setup_idt() is told to handle #DB,
> > 
> > Hmm, it might be a moot point for this patch (see below), but my vote is to have
> > setup_idt() wire up all known handlers to check_exception_table().  I don't see
> > any reason to skip some vectors.  Code with __ASM_TRY() will explode no matter what,
> > so it's not like it risks suppressing completely unexpected faults.
> 
> I would just like to point out that this whole FEP approach is not friendly
> for other hypervisors or bare-metal testings. While people might have mixed
> feelings about other hypervisors, the benefit of running the tests on
> bare-metal should be clear.

If people want to run KUT on other hypervisors that support a different FEP (KVM
stole it from Xen after all), then I have no objection to genericizing KVM_FEP
and making it a config option.

> Specifically in this test, it is rather simple to avoid using FEP.

I'm saying KUT should do both, i.e. run the same sequences with and without
forced emulation on the "critical" instructions.  That provides full coverage for
bare metal, and also provides coverage for both "fast" and "slow" emulation in
KVM (and other future hypervisors if other prefixes are ever supported).

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Michal Luczaj]

On 8/22/22 17:42, Sean Christopherson wrote:
>> On 8/22/22 00:06, Michal Luczaj wrote:
>> test can be simplified to something like
>>
>> 	asm volatile("push %[ss]\n\t"
>> 		     __ASM_TRY(KVM_FEP "pop %%ss", "1f")
>> 		     "ex_blocked: mov $1, %[success]\n\t"
> 
> So I'm 99% certain this only passes because KVM doesn't emulate the `mov $1, %[success]`.
> kvm_vcpu_check_code_breakpoint() honors EFLAGS.RF, but not MOV/POP_SS blocking.
> I.e. I would expect this to fail if the `mov` were also tagged KVM_FEP.

I wasn't sure if I understood you correctly, so, FWIW, I've ran:

static void test_pop_ss_blocking_try(void)
{
	int success;

	write_dr7(DR7_FIXED_1 |
		  DR7_GLOBAL_ENABLE_DRx(0) |
		  DR7_EXECUTE_DRx(0) |
		  DR7_LEN_1_DRx(0));

#define POPSS(desc, fep1, fep2)					\
	asm volatile("mov $0, %[success]\n\t"			\
		     "lea 1f, %%eax\n\r"			\
		     "mov %%eax, %%dr0\n\r"			\
		     "push %%ss\n\t"				\
		     __ASM_TRY(fep1 "pop %%ss", "2f")		\
		     "1:" fep2 "mov $1, %[success]\n\t"		\
		     "2:"					\
		     : [success] "=g" (success)			\
		     :						\
		     : "eax", "memory");			\
	report(success, desc ": #DB suppressed after POP SS")

	POPSS("no fep", "", "");
	POPSS("fep pop-ss", KVM_FEP, "");
	POPSS("fep mov-1", "", KVM_FEP);
	POPSS("fep pop-ss/fep mov-1", KVM_FEP, KVM_FEP);

	write_dr7(DR7_FIXED_1);
}

and got:

em_pop_sreg unpatched:
PASS: no fep: #DB suppressed after POP SS
FAIL: fep pop-ss: #DB suppressed after POP SS
PASS: fep mov-1: #DB suppressed after POP SS
FAIL: fep pop-ss/fep mov-1: #DB suppressed after POP SS

em_pop_sreg patched:
PASS: no fep: #DB suppressed after POP SS
PASS: fep pop-ss: #DB suppressed after POP SS
PASS: fep mov-1: #DB suppressed after POP SS
PASS: fep pop-ss/fep mov-1: #DB suppressed after POP SS

For the sake of completeness: basically the same, but MOV SS, i.e.

	asm volatile("mov $0, %[success]\n\t"			\
		     "lea 1f, %%eax\n\r"			\
		     "mov %%eax, %%dr0\n\r"			\
		     "mov %%ss, %%eax\n\t"			\
		     __ASM_TRY(fep1 "mov %%eax, %%ss", "2f")	\
		     "1:" fep2 "mov $1, %[success]\n\t"		\
		     "2:"					\
		     : [success] "=g" (success)			\
		     :						\
		     : "eax", "memory");			\

PASS: no fep: #DB suppressed after MOV SS
PASS: fep mov-ss: #DB suppressed after MOV SS
PASS: fep mov-1: #DB suppressed after MOV SS
PASS: fep mov-ss/fep mov-1: #DB suppressed after MOV SS

> The reason I say the setup_idt() change is a moot point is because I think it
> would be better to put this testcase into debug.c (where "this" testcase is really
> going to be multiple testcases).  With macro shenanigans (or code patching), it
> should be fairly easy to run all testcases with both FEP=0 and FEP=1.
>
> Then it's "just" a matter of adding a code #DB testcase.  __run_single_step_db_test()
> and run_ss_db_test() aren't fundamentally tied to single-step, i.e. can simply be
> renamed.  For simplicity, I'd say just skip the usermode test for code #DBs, IMO
> it's not worth the extra coverage.

So that would involve a 32-bit segment for POP SS and making use of DR0/DR7 instead
of single stepping? I guess I can try.

Thanks,
Michal

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Sean Christopherson]

On Tue, Aug 23, 2022, Michal Luczaj wrote:
> On 8/22/22 17:42, Sean Christopherson wrote:
> >> On 8/22/22 00:06, Michal Luczaj wrote:
> >> test can be simplified to something like
> >>
> >> 	asm volatile("push %[ss]\n\t"
> >> 		     __ASM_TRY(KVM_FEP "pop %%ss", "1f")
> >> 		     "ex_blocked: mov $1, %[success]\n\t"
> > 
> > So I'm 99% certain this only passes because KVM doesn't emulate the `mov $1, %[success]`.
> > kvm_vcpu_check_code_breakpoint() honors EFLAGS.RF, but not MOV/POP_SS blocking.
> > I.e. I would expect this to fail if the `mov` were also tagged KVM_FEP.
> 
> I wasn't sure if I understood you correctly

Yep, you got the gist.

> so, FWIW, I've ran:
> 
> static void test_pop_ss_blocking_try(void)
> {
> 	int success;
> 
> 	write_dr7(DR7_FIXED_1 |
> 		  DR7_GLOBAL_ENABLE_DRx(0) |
> 		  DR7_EXECUTE_DRx(0) |
> 		  DR7_LEN_1_DRx(0));
> 
> #define POPSS(desc, fep1, fep2)					\
> 	asm volatile("mov $0, %[success]\n\t"			\
> 		     "lea 1f, %%eax\n\r"			\

Note, hardcoding EAX doesn't compile for 64-bit KUT.  It's kinda gross, but we
can fudge around this without #ifdeffery by using a dummy input constraint.

> 		     "mov %%eax, %%dr0\n\r"			\
> 		     "push %%ss\n\t"				\
> 		     __ASM_TRY(fep1 "pop %%ss", "2f")		\

No need for __ASM_TRY if this is thrown into debug.c.

> 		     "1:" fep2 "mov $1, %[success]\n\t"		\
> 		     "2:"					\
> 		     : [success] "=g" (success)			\
> 		     :						\
> 		     : "eax", "memory");			\
> 	report(success, desc ": #DB suppressed after POP SS")

To avoid potential "leakage", wrap this whole thing in ({ ... }), and declare
"success" inside that scope.

Aha!  Even better.  s/success/r, make it unsigned long, and force it to be a
register.  Then the blob can use the register as scratch before clearing it via
XOR to indicate success (though that's somewhat pointless, see below).

> 	POPSS("no fep", "", "");
> 	POPSS("fep pop-ss", KVM_FEP, "");
> 	POPSS("fep mov-1", "", KVM_FEP);
> 	POPSS("fep pop-ss/fep mov-1", KVM_FEP, KVM_FEP);
> 
> 	write_dr7(DR7_FIXED_1);
> }
> 
> and got:
> 
> em_pop_sreg unpatched:
> PASS: no fep: #DB suppressed after POP SS
> FAIL: fep pop-ss: #DB suppressed after POP SS
> PASS: fep mov-1: #DB suppressed after POP SS
> FAIL: fep pop-ss/fep mov-1: #DB suppressed after POP SS
> 
> em_pop_sreg patched:
> PASS: no fep: #DB suppressed after POP SS
> PASS: fep pop-ss: #DB suppressed after POP SS
> PASS: fep mov-1: #DB suppressed after POP SS
> PASS: fep pop-ss/fep mov-1: #DB suppressed after POP SS
> 
> For the sake of completeness: basically the same, but MOV SS, i.e.
> 
> 	asm volatile("mov $0, %[success]\n\t"			\
> 		     "lea 1f, %%eax\n\r"			\
> 		     "mov %%eax, %%dr0\n\r"			\
> 		     "mov %%ss, %%eax\n\t"			\
> 		     __ASM_TRY(fep1 "mov %%eax, %%ss", "2f")	\
> 		     "1:" fep2 "mov $1, %[success]\n\t"		\
> 		     "2:"					\
> 		     : [success] "=g" (success)			\
> 		     :						\
> 		     : "eax", "memory");			\
> 
> PASS: no fep: #DB suppressed after MOV SS
> PASS: fep mov-ss: #DB suppressed after MOV SS
> PASS: fep mov-1: #DB suppressed after MOV SS
> PASS: fep mov-ss/fep mov-1: #DB suppressed after MOV SS

This passes for 3 reasons:

  1. The test more than likely doesn't skip the instruction on #DB, and so
     success will be set regardless of whether or not a #DB occurred.

  2. DR0 needs to be loaded with the address of the MOV, not the address of the
     FEP prefix.  Somewhat amusingly, this means my patch to fix redundant #DB
     checking is wrong[*]

  3. My personal favorite.  On VM-Exits due to exceptions (#UD), Intel CPUs set
     EFLAGS.RF=1 and thus effectively suppress #DBs on FEP instructions.

       If the VM exit is caused directly by an event that would normally be delivered
       through the IDT, the value saved is that which would appear in the saved RFLAGS
       image (either that which would be saved on the stack had the event been delivered
       through a trap or interrupt gate1 or into the old task-state segment had the event
       been delivered through a task gate) had the event been delivered through the IDT.
       See below for VM exits due to task switches caused by task gates in the IDT.

     I'm pretty sure this inarguably a KVM bug.  The SDM states the EFLAGS.RF=0
     on instruction-based VM-Exits, and since the intent in the FEP case (but not
     the general #UD case) is to simulate an instruction exit...

       If the VM exit is caused by an attempt to execute an instruction that
       unconditionally causes VM exits or one that was configured to do with a
       VM-execution control, the value saved is 0.

I'll fold a fix for #3 and for the MOV/POP-SS blocking code #DB interaction into
the aforementioned series[*].

As expected, I get two failures (FEP on the XOR testcases) with this tweak to KVM,

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fb5cecb19cf5..e5fd0b936a48 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7261,6 +7261,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
            kvm_read_guest_virt(vcpu, kvm_get_linear_rip(vcpu),
                                sig, sizeof(sig), &e) == 0 &&
            memcmp(sig, kvm_emulate_prefix, sizeof(sig)) == 0) {
+               kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) & ~X86_EFLAGS_RF);
                kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
                emul_type = EMULTYPE_TRAP_UD_FORCED;
        }

and this as a testcase.

diff --git a/x86/debug.c b/x86/debug.c
index b66bf047..ab29e94e 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -352,8 +352,44 @@ static unsigned long singlestep_with_movss_blocking_and_dr7_gd(void)
        return start_rip;
 }
 
+static void test_mov_ss_code_db(bool fep_available)
+{
+       write_dr7(DR7_FIXED_1 |
+                 DR7_GLOBAL_ENABLE_DRx(0) |
+                 DR7_EXECUTE_DRx(0) |
+                 DR7_LEN_1_DRx(0));
+
+#define MOVSS_DB(desc, fep1, fep2)                             \
+({                                                             \
+       unsigned long r;                                        \
+                                                               \
+       n = 0;                                                  \
+       asm volatile("lea 1f(%%rip), %0\n\t"                    \
+                    "mov %0, %%dr0\n\t"                        \
+                    "mov %%ss, %0\n\t"                         \
+                    fep1 "mov  %0, %%ss\n\t"                   \
+                    fep2 "1: xor %0, %0\n\t"                   \
+                    "2:"                                       \
+                    : "=r" (r)                                 \
+                    :                                          \
+                    : "memory");                               \
+       report(!n && !r, desc ": #DB suppressed after MOV SS"); \
+})
+
+       MOVSS_DB("no fep", "", "");
+       if (fep_available) {
+               MOVSS_DB("fep MOV-SS", KVM_FEP, "");
+               MOVSS_DB("fep XOR", "", KVM_FEP);
+               MOVSS_DB("fep MOV-SS/fep XOR", KVM_FEP, KVM_FEP);
+       }
+
+       write_dr7(DR7_FIXED_1);
+}
+
 int main(int ac, char **av)
 {
+       /* Check for KVM's FEP support prior to usurpsing the #UD handler. */
+       bool fep_available = is_fep_available();
        unsigned long cr4;
 
        handle_exception(DB_VECTOR, handle_db);
@@ -417,6 +453,8 @@ int main(int ac, char **av)
        run_ss_db_test(singlestep_with_movss_blocking_and_icebp);
        run_ss_db_test(singlestep_with_movss_blocking_and_dr7_gd);
 
+       test_mov_ss_code_db(fep_available);
+
        n = 0;
        write_dr1((void *)&value);
        write_dr6(DR6_BS);


[*] https://lore.kernel.org/all/20220723005137.1649592-4-seanjc@google.com

> > The reason I say the setup_idt() change is a moot point is because I think it
> > would be better to put this testcase into debug.c (where "this" testcase is really
> > going to be multiple testcases).  With macro shenanigans (or code patching), it
> > should be fairly easy to run all testcases with both FEP=0 and FEP=1.
> >
> > Then it's "just" a matter of adding a code #DB testcase.  __run_single_step_db_test()
> > and run_ss_db_test() aren't fundamentally tied to single-step, i.e. can simply be
> > renamed.  For simplicity, I'd say just skip the usermode test for code #DBs, IMO
> > it's not worth the extra coverage.
> 
> So that would involve a 32-bit segment for POP SS and making use of DR0/DR7 instead
> of single stepping? I guess I can try.

Eh, let's completely skip usermode for code #DBs and not tweak __run_single_step_db_test().
It's easier to just have a standalone function.

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Michal Luczaj]

On 8/24/22 20:32, Sean Christopherson wrote:
> Eh, let's completely skip usermode for code #DBs and not tweak __run_single_step_db_test().
> It's easier to just have a standalone function.

Something like this?

static void test_pop_ss_code_db(bool fep_available)
{
	write_ss(KERNEL_DS);

	write_dr7(DR7_FIXED_1 |
		  DR7_ENABLE_DRx(0) |
		  DR7_EXECUTE_DRx(0) |
		  DR7_LEN_1_DRx(0));

#define POPSS_DB(desc, fep1, fep2)				\
({								\
	unsigned int r;						\
								\
	n = 0;							\
	asm volatile(/* jump to 32-bit code segment */		\
		     "ljmp *1f\n\t"				\
		     "1:\n\t"					\
		     "	.long 2f\n\t"				\
		     "	.word " xstr(KERNEL_CS32) "\n\t"	\
		     /* exercise POP SS blocking */		\
		     ".code32\n\t"				\
		     "2: lea 3f, %0\n\t"			\
		     "mov %0, %%dr0\n\t"			\
		     "push %%ss\n\t"				\
		     fep1 "pop %%ss\n\t"			\
		     fep2 "3: xor %0, %0\n\t"			\
		     /* back to long mode */			\
		     "ljmp %[cs64], $4f\n\t"			\
		     ".code64\n\t"				\
		     "4:"					\
		     : "=R" (r)					\
		     : [cs64] "i" (KERNEL_CS64)			\
		     : "memory");				\
								\
	report(!n && !r, desc ": #DB suppressed after POP SS"); \
})

	POPSS_DB("no fep", "", "");
	if (fep_available) {
		POPSS_DB("fep POP-SS", KVM_FEP, "");
		POPSS_DB("fep XOR", "", KVM_FEP);
		POPSS_DB("fep POP-SS/fep XOR", KVM_FEP, KVM_FEP);
	}

	write_dr7(DR7_FIXED_1);
}

Results:

em_pop_sreg unpatched
PASS: no fep: #DB suppressed after POP SS
FAIL: fep POP-SS: #DB suppressed after POP SS
PASS: fep XOR: #DB suppressed after POP SS
PASS: fep POP-SS/fep XOR: #DB suppressed after POP SS

em_pop_sreg patched
PASS: no fep: #DB suppressed after POP SS
PASS: fep POP-SS: #DB suppressed after POP SS
PASS: fep XOR: #DB suppressed after POP SS
PASS: fep POP-SS/fep XOR: #DB suppressed after POP SS

thanks,
Michal

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Sean Christopherson]

On Wed, Aug 24, 2022, Michal Luczaj wrote:
> On 8/24/22 20:32, Sean Christopherson wrote:
> > Eh, let's completely skip usermode for code #DBs and not tweak __run_single_step_db_test().
> > It's easier to just have a standalone function.
> 
> Something like this?
> 
> static void test_pop_ss_code_db(bool fep_available)
> {
> 	write_ss(KERNEL_DS);
> 
> 	write_dr7(DR7_FIXED_1 |
> 		  DR7_ENABLE_DRx(0) |
> 		  DR7_EXECUTE_DRx(0) |
> 		  DR7_LEN_1_DRx(0));
> 
> #define POPSS_DB(desc, fep1, fep2)				\
> ({								\
> 	unsigned int r;						\
> 								\
> 	n = 0;							\
> 	asm volatile(/* jump to 32-bit code segment */		\
> 		     "ljmp *1f\n\t"				\
> 		     "1:\n\t"					\
> 		     "	.long 2f\n\t"				\
> 		     "	.word " xstr(KERNEL_CS32) "\n\t"	\
> 		     /* exercise POP SS blocking */		\
> 		     ".code32\n\t"				\
> 		     "2: lea 3f, %0\n\t"			\
> 		     "mov %0, %%dr0\n\t"			\
> 		     "push %%ss\n\t"				\
> 		     fep1 "pop %%ss\n\t"			\
> 		     fep2 "3: xor %0, %0\n\t"			\
> 		     /* back to long mode */			\
> 		     "ljmp %[cs64], $4f\n\t"			\
> 		     ".code64\n\t"				\

Ooh, I see what you meant by temporarily switching to 32-bit mode.  I was thinking
we could just make the POP SS testcase 32-bit only, but I didn't realize this test
is 64-bit only.  Argh, and so is emulate.c.  And now I get why you added a brand
new test.

Let's just add a new test.  The above can work, but it relies on the code and
stack being mapped with a 32-bit address, e.g. will break if KUT is ever changed
to not map everything low in the virtual address space.

I think it makes sense to rename emulator.c => emulator64.c, and then start a new
emulator.c for tests that apply to both 32-bit and 64-bit KUT.

I'll send a small series, the behavior is also different for AMD CPUs (I coded up
99% of this yesterday before realizing this morning that debug.c is 64-bit only).

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Michal Luczaj]

On 8/25/22 19:03, Sean Christopherson wrote:
> On Wed, Aug 24, 2022, Michal Luczaj wrote:
>> 								\
>> 	n = 0;							\
>> 	asm volatile(/* jump to 32-bit code segment */		\
>> 		     "ljmp *1f\n\t"				\
>> 		     "1:\n\t"					\
>> 		     "	.long 2f\n\t"				\
>> 		     "	.word " xstr(KERNEL_CS32) "\n\t"	\
>> 		     /* exercise POP SS blocking */		\
>> 		     ".code32\n\t"				\
>> 		     "2: lea 3f, %0\n\t"			\
>> 		     "mov %0, %%dr0\n\t"			\
>> 		     "push %%ss\n\t"				\
>> 		     fep1 "pop %%ss\n\t"			\
>> 		     fep2 "3: xor %0, %0\n\t"			\
>> 		     /* back to long mode */			\
>> 		     "ljmp %[cs64], $4f\n\t"			\
>> 		     ".code64\n\t"				\
> 
> Ooh, I see what you meant by temporarily switching to 32-bit mode.  I was thinking
> we could just make the POP SS testcase 32-bit only, but I didn't realize this test
> is 64-bit only.  Argh, and so is emulate.c.  And now I get why you added a brand
> new test.
>
> Let's just add a new test.  The above can work, but it relies on the code and
> stack being mapped with a 32-bit address, e.g. will break if KUT is ever changed
> to not map everything low in the virtual address space.

Yeah, it is fragile in that sense. But does it mean code such as
vmx_tests.c:into_guest_main() or svm_tests.c:svm_of_test_guest() should be moved
to 32-bit binaries?

Michal

Re: [kvm-unit-tests PATCH] x86/emulator: Test POP-SS blocking

[Sean Christopherson]

On Thu, Aug 25, 2022, Michal Luczaj wrote:
> On 8/25/22 19:03, Sean Christopherson wrote:
> > On Wed, Aug 24, 2022, Michal Luczaj wrote:
> >> 								\
> >> 	n = 0;							\
> >> 	asm volatile(/* jump to 32-bit code segment */		\
> >> 		     "ljmp *1f\n\t"				\
> >> 		     "1:\n\t"					\
> >> 		     "	.long 2f\n\t"				\
> >> 		     "	.word " xstr(KERNEL_CS32) "\n\t"	\
> >> 		     /* exercise POP SS blocking */		\
> >> 		     ".code32\n\t"				\
> >> 		     "2: lea 3f, %0\n\t"			\
> >> 		     "mov %0, %%dr0\n\t"			\
> >> 		     "push %%ss\n\t"				\
> >> 		     fep1 "pop %%ss\n\t"			\
> >> 		     fep2 "3: xor %0, %0\n\t"			\
> >> 		     /* back to long mode */			\
> >> 		     "ljmp %[cs64], $4f\n\t"			\
> >> 		     ".code64\n\t"				\
> > 
> > Ooh, I see what you meant by temporarily switching to 32-bit mode.  I was thinking
> > we could just make the POP SS testcase 32-bit only, but I didn't realize this test
> > is 64-bit only.  Argh, and so is emulate.c.  And now I get why you added a brand
> > new test.
> >
> > Let's just add a new test.  The above can work, but it relies on the code and
> > stack being mapped with a 32-bit address, e.g. will break if KUT is ever changed
> > to not map everything low in the virtual address space.
> 
> Yeah, it is fragile in that sense. But does it mean code such as
> vmx_tests.c:into_guest_main() or svm_tests.c:svm_of_test_guest() should be moved
> to 32-bit binaries?

Ideally?  Yeah.  In practice, it's just not worth replicating all the nSVM/nVMX
setup for 32-bit binaries.

Re: [PATCH] KVM: x86/emulator: Fix handing of POP SS to correctly set interruptibility

[Sean Christopherson]

On Sun, Aug 21, 2022, Michal Luczaj wrote:
> The emulator checks the wrong variable while setting the CPU
> interruptibility state.  Fix the condition.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

  Fixes: a5457e7bcf9a ("KVM: emulate: POP SS triggers a MOV SS shadow too")

and probably 

  Cc: stable@vger.kernel.org

even though I'd be amazed if this actually fixes anyone's workloads :-)

Reviewed-by: Sean Christopherson <seanjc@google.com>


Paolo, do you want to grab this for 6.0, or should I throw it in the queue for 6.1?

Re: [PATCH] KVM: x86/emulator: Fix handing of POP SS to correctly set interruptibility

[Paolo Bonzini]

On 8/24/22 02:20, Sean Christopherson wrote:
>    Fixes: a5457e7bcf9a ("KVM: emulate: POP SS triggers a MOV SS shadow too")
> 
> and probably
> 
>    Cc:stable@vger.kernel.org
> 
> even though I'd be amazed if this actually fixes anyone's workloads:-)
> 
> Reviewed-by: Sean Christopherson<seanjc@google.com>
> 
> 
> Paolo, do you want to grab this for 6.0, or should I throw it in the queue for 6.1?

Go ahead for 6.1.

Paolo

Re: [PATCH] KVM: x86/emulator: Fix handing of POP SS to correctly set interruptibility

[Sean Christopherson]

On Sun, Aug 21, 2022, Michal Luczaj wrote:
> The emulator checks the wrong variable while setting the CPU
> interruptibility state.  Fix the condition.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

Pushed to branch `for_paolo/6.1` at:

    https://github.com/sean-jc/linux.git

Unless you hear otherwise, it will make its way to kvm/queue "soon".

Note, the commit IDs are not guaranteed to be stable.