[PATCH 0/2] KVM: x86: Rep string I/O WARN removal and test

[PATCH 0/2] KVM: x86: Rep string I/O WARN removal and test

[Sean Christopherson]

Remove a WARN that was added as part of the recent I/O overhaul to play
nice with SEV-ES string I/O.

For the record, my FIXME in lieu of a WARN was deliberate, as I suspected
userspace could trigger a WARN ;-)

Based on kvm/master, commit 95e16b4792b0 ("KVM: SEV-ES: go over the
sev_pio_data buffer in multiple passes if needed").

Sean Christopherson (2):
  KVM: x86: Don't WARN if userspace mucks with RCX during string I/O
    exit
  KVM: selftests: Add test to verify KVM doesn't explode on "bad" I/O

 arch/x86/kvm/x86.c                            |   9 +-
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/userspace_io_test.c  | 114 ++++++++++++++++++
 4 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/userspace_io_test.c

-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 1/2] KVM: x86: Don't WARN if userspace mucks with RCX during string I/O exit

[Sean Christopherson]

Replace a WARN with a comment to call out that userspace can modify RCX
during an exit to userspace to handle string I/O.  KVM doesn't actually
support changing the rep count during an exit, i.e. the scenario can be
ignored, but the WARN needs to go as it's trivial to trigger from
userspace.

Cc: stable@vger.kernel.org
Fixes: 3b27de271839 ("KVM: x86: split the two parts of emulator_pio_in")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b26647a5ea22..1726812f31af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6948,7 +6948,13 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 			   unsigned short port, void *val, unsigned int count)
 {
 	if (vcpu->arch.pio.count) {
-		/* Complete previous iteration.  */
+		/*
+		 * Complete a previous iteration that required userspace I/O.
+		 * Note, @count isn't guaranteed to match pio.count as userspace
+		 * can modify ECX before rerunning the vCPU.  Ignore any such
+		 * shenanigans as KVM doesn't support modifying the rep count,
+		 * and the emulator ensures @count doesn't overflow the buffer.
+		 */
 	} else {
 		int r = __emulator_pio_in(vcpu, size, port, count);
 		if (!r)
@@ -6957,7 +6963,6 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
 		/* Results already available, fall through.  */
 	}
 
-	WARN_ON(count != vcpu->arch.pio.count);
 	complete_emulator_pio_in(vcpu, val);
 	return 1;
 }
-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 2/2] KVM: selftests: Add test to verify KVM doesn't explode on "bad" I/O

[Sean Christopherson]

Add an x86 selftest to verify that KVM doesn't WARN or otherwise explode
if userspace modifies RCX during a userspace exit to handle string I/O.
This is a regression test for a user-triggerable WARN introduced by
commit 3b27de271839 ("KVM: x86: split the two parts of emulator_pio_in").

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/userspace_io_test.c  | 114 ++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/userspace_io_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index b8dbabe24ac2..691262c7b122 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -27,6 +27,7 @@
 /x86_64/svm_int_ctl_test
 /x86_64/sync_regs_test
 /x86_64/tsc_msrs_test
+/x86_64/userspace_io_test
 /x86_64/userspace_msr_exit_test
 /x86_64/vmx_apic_access_test
 /x86_64/vmx_close_while_nested_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index d1774f461393..1a611cb80c4e 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -58,6 +58,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
 TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
 TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test
 TEST_GEN_PROGS_x86_64 += x86_64/sync_regs_test
+TEST_GEN_PROGS_x86_64 += x86_64/userspace_io_test
 TEST_GEN_PROGS_x86_64 += x86_64/userspace_msr_exit_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_apic_access_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_close_while_nested_test
diff --git a/tools/testing/selftests/kvm/x86_64/userspace_io_test.c b/tools/testing/selftests/kvm/x86_64/userspace_io_test.c
new file mode 100644
index 000000000000..e4bef2e05686
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/userspace_io_test.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+
+#define VCPU_ID			1
+
+static void guest_ins_port80(uint8_t *buffer, unsigned int count)
+{
+	unsigned long end;
+
+	if (count == 2)
+		end = (unsigned long)buffer + 1;
+	else
+		end = (unsigned long)buffer + 8192;
+
+	asm volatile("cld; rep; insb" : "+D"(buffer), "+c"(count) : "d"(0x80) : "memory");
+	GUEST_ASSERT_1(count == 0, count);
+	GUEST_ASSERT_2((unsigned long)buffer == end, buffer, end);
+}
+
+static void guest_code(void)
+{
+	uint8_t buffer[8192];
+	int i;
+
+	/*
+	 * Special case tests.  main() will adjust RCX 2 => 1 and 3 => 8192 to
+	 * test that KVM doesn't explode when userspace modifies the "count" on
+	 * a userspace I/O exit.  KVM isn't required to play nice with the I/O
+	 * itself as KVM doesn't support manipulating the count, it just needs
+	 * to not explode or overflow a buffer.
+	 */
+	guest_ins_port80(buffer, 2);
+	guest_ins_port80(buffer, 3);
+
+	/* Verify KVM fills the buffer correctly when not stuffing RCX. */
+	memset(buffer, 0, sizeof(buffer));
+	guest_ins_port80(buffer, 8192);
+	for (i = 0; i < 8192; i++)
+		GUEST_ASSERT_2(buffer[i] == 0xaa, i, buffer[i]);
+
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_regs regs;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	int rc;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	/* Create VM */
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+	run = vcpu_state(vm, VCPU_ID);
+
+	memset(&regs, 0, sizeof(regs));
+
+	while (1) {
+		rc = _vcpu_run(vm, VCPU_ID);
+
+		TEST_ASSERT(rc == 0, "vcpu_run failed: %d\n", rc);
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Unexpected exit reason: %u (%s),\n",
+			    run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		if (get_ucall(vm, VCPU_ID, &uc))
+			break;
+
+		TEST_ASSERT(run->io.port == 0x80,
+			    "Expected I/O at port 0x80, got port 0x%x\n", run->io.port);
+
+		/*
+		 * Modify the rep string count in RCX: 2 => 1 and 3 => 8192.
+		 * Note, this abuses KVM's batching of rep string I/O to avoid
+		 * getting stuck in an infinite loop.  That behavior isn't in
+		 * scope from a testing perspective as it's not ABI in any way,
+		 * i.e. it really is abusing internal KVM knowledge.
+		 */
+		vcpu_regs_get(vm, VCPU_ID, &regs);
+		if (regs.rcx == 2)
+			regs.rcx = 1;
+		if (regs.rcx == 3)
+			regs.rcx = 8192;
+		memset((void *)run + run->io.data_offset, 0xaa, 4096);
+		vcpu_regs_set(vm, VCPU_ID, &regs);
+	}
+
+	switch (uc.cmd) {
+	case UCALL_DONE:
+		break;
+	case UCALL_ABORT:
+		TEST_FAIL("%s at %s:%ld : argN+1 = 0x%lx, argN+2 = 0x%lx",
+			  (const char *)uc.args[0], __FILE__, uc.args[1],
+			  uc.args[2], uc.args[3]);
+	default:
+		TEST_FAIL("Unknown ucall %lu", uc.cmd);
+	}
+
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 0/2] KVM: x86: Rep string I/O WARN removal and test

[Sean Christopherson]

On Mon, Oct 25, 2021, Sean Christopherson wrote:
> Remove a WARN that was added as part of the recent I/O overhaul to play
> nice with SEV-ES string I/O.
> 
> For the record, my FIXME in lieu of a WARN was deliberate, as I suspected
> userspace could trigger a WARN ;-)
> 
> Based on kvm/master, commit 95e16b4792b0 ("KVM: SEV-ES: go over the
> sev_pio_data buffer in multiple passes if needed").
> 
> Sean Christopherson (2):
>   KVM: x86: Don't WARN if userspace mucks with RCX during string I/O
>     exit
>   KVM: selftests: Add test to verify KVM doesn't explode on "bad" I/O
> 
>  arch/x86/kvm/x86.c                            |   9 +-
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/x86_64/userspace_io_test.c  | 114 ++++++++++++++++++
>  4 files changed, 123 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/userspace_io_test.c

Ping.  I completely forgot about this too, until I unintentionally ran a
userspace_io_test that was lying around.

Re: [PATCH 0/2] KVM: x86: Rep string I/O WARN removal and test

[Paolo Bonzini]

On 12/9/21 06:48, Sean Christopherson wrote:
> On Mon, Oct 25, 2021, Sean Christopherson wrote:
>> Remove a WARN that was added as part of the recent I/O overhaul to play
>> nice with SEV-ES string I/O.
>>
>> For the record, my FIXME in lieu of a WARN was deliberate, as I suspected
>> userspace could trigger a WARN ;-)
>>
>> Based on kvm/master, commit 95e16b4792b0 ("KVM: SEV-ES: go over the
>> sev_pio_data buffer in multiple passes if needed").
>>
>> Sean Christopherson (2):
>>    KVM: x86: Don't WARN if userspace mucks with RCX during string I/O
>>      exit
>>    KVM: selftests: Add test to verify KVM doesn't explode on "bad" I/O
>>
>>   arch/x86/kvm/x86.c                            |   9 +-
>>   tools/testing/selftests/kvm/.gitignore        |   1 +
>>   tools/testing/selftests/kvm/Makefile          |   1 +
>>   .../selftests/kvm/x86_64/userspace_io_test.c  | 114 ++++++++++++++++++
>>   4 files changed, 123 insertions(+), 2 deletions(-)
>>   create mode 100644 tools/testing/selftests/kvm/x86_64/userspace_io_test.c
> 
> Ping.  I completely forgot about this too, until I unintentionally ran a
> userspace_io_test that was lying around.
> 

Queued now, thanks.  I don't know if I want the honor of having KVM 
singled out again on the -rc release message, but these are bugs 
nevertheless...

Paolo

Re: [PATCH 0/2] KVM: x86: Rep string I/O WARN removal and test

[Sean Christopherson]

On Thu, Dec 09, 2021, Paolo Bonzini wrote:
> On 12/9/21 06:48, Sean Christopherson wrote:
> > On Mon, Oct 25, 2021, Sean Christopherson wrote:
> > > Remove a WARN that was added as part of the recent I/O overhaul to play
> > > nice with SEV-ES string I/O.
> > > 
> > > For the record, my FIXME in lieu of a WARN was deliberate, as I suspected
> > > userspace could trigger a WARN ;-)
> > > 
> > > Based on kvm/master, commit 95e16b4792b0 ("KVM: SEV-ES: go over the
> > > sev_pio_data buffer in multiple passes if needed").
> > > 
> > > Sean Christopherson (2):
> > >    KVM: x86: Don't WARN if userspace mucks with RCX during string I/O
> > >      exit
> > >    KVM: selftests: Add test to verify KVM doesn't explode on "bad" I/O
> > > 
> > >   arch/x86/kvm/x86.c                            |   9 +-
> > >   tools/testing/selftests/kvm/.gitignore        |   1 +
> > >   tools/testing/selftests/kvm/Makefile          |   1 +
> > >   .../selftests/kvm/x86_64/userspace_io_test.c  | 114 ++++++++++++++++++
> > >   4 files changed, 123 insertions(+), 2 deletions(-)
> > >   create mode 100644 tools/testing/selftests/kvm/x86_64/userspace_io_test.c
> > 
> > Ping.  I completely forgot about this too, until I unintentionally ran a
> > userspace_io_test that was lying around.
> > 
> 
> Queued now, thanks.  I don't know if I want the honor of having KVM singled
> out again on the -rc release message, but these are bugs nevertheless...

Heh, you could always wait an -rc to send the fixes and hope Linus has a short
memory :-D