[PATCH 0/2] KVM: nVMX: Don't clobber VM-Entry instruction length on soft INTn exits

[PATCH 0/2] KVM: nVMX: Don't clobber VM-Entry instruction length on soft INTn exits

[ma.mandourr]

From: Mahmoud Abumandour <ma.mandourr@gmail.com>

KVM writes the length of a soft INTn that L2 was vectoring at the time of
a nested VM-Exit into vmcs12's VM-Entry instruction length, an L1-owned
control, instead of the VM-Exit instruction length.  Patch 1 fixes it and
patch 2 adds a selftest.

Found with a fuzzer for VMX isolation. The selftest patch was written with
the help of an LLM coding assistant (Claude Opus 5.5). kvm-x86/next was
run as a nested hypervisor under QEMU. The selftest passes only with
patch 1, and other nested VMX selftests are unaffected.

Mahmoud Abumandour (2):
  KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft
    INTn
  KVM: selftests: Add test for nested VM-Exits while vectoring soft INTs

 arch/x86/kvm/vmx/nested.c                     |   2 +-
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../kvm/x86/vmx_soft_int_vectoring_test.c     | 105 ++++++++++++++++++
 3 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c


base-commit: dea4f6380f4ec96e8f3b0aaf2343764bd0d25ef4
-- 
2.43.0

[PATCH 1/2] KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft INTn

[ma.mandourr]

From: Mahmoud Abumandour <ma.mandourr@gmail.com>

When a VM-Exit from L2 occurs while vectoring a software interrupt, write
the instruction length to vmcs12's VM-Exit instruction length field, as is
done for software exceptions, not to the VM-Entry instruction length field,
which belongs to L1.

Found with a differential testing harness for VMX isolation.

Fixes: 5f3d5799974b ("KVM: nVMX: Rework event injection and recovery")
Signed-off-by: Mahmoud Abumandour <ma.mandourr@gmail.com>
---
 arch/x86/kvm/vmx/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 9b0bfa2f854c..302f5b5aa582 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4016,7 +4016,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
 
 		if (vcpu->arch.interrupt.soft) {
 			idt_vectoring |= INTR_TYPE_SOFT_INTR;
-			vmcs12->vm_entry_instruction_len =
+			vmcs12->vm_exit_instruction_len =
 				vcpu->arch.event_exit_inst_len;
 		} else
 			idt_vectoring |= INTR_TYPE_EXT_INTR;
-- 
2.43.0

Re: [PATCH 1/2] KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft INTn

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: nVMX: vm_exit_instruction_len overwritten in prepare_vmcs12()
--

commit 24bdb8dd4aa6675d8e53850c4d0acf71d5903e75
Author: Mahmoud Abumandour <ma.mandourr@gmail.com>

KVM: nVMX: Don't clobber VM-Entry instruction length when saving soft INTn

This patch updates vmcs12_save_pending_event() so that when a nested VM-Exit
occurs while vectoring a software interrupt, vcpu->arch.event_exit_inst_len is
written to vmcs12->vm_exit_instruction_len instead of
vmcs12->vm_entry_instruction_len.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 9b0bfa2f854cf..302f5b5aa5823 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4016,7 +4016,7 @@ static void vmcs12_save_pending_event(struct kvm_vcpu *vcpu,
>  
>  		if (vcpu->arch.interrupt.soft) {
>  			idt_vectoring |= INTR_TYPE_SOFT_INTR;
> -			vmcs12->vm_entry_instruction_len =
> +			vmcs12->vm_exit_instruction_len =
>  				vcpu->arch.event_exit_inst_len;
>  		} else
>  			idt_vectoring |= INTR_TYPE_EXT_INTR;

[Severity: Medium]
Does writing vmcs12->vm_exit_instruction_len in vmcs12_save_pending_event()
actually take effect?

prepare_vmcs12() in arch/x86/kvm/vmx/nested.c is the sole caller of
vmcs12_save_pending_event(), and since commit c3634d25fbee ("KVM: nVMX: Leave
most VM-Exit info fields unmodified on failed VM-Entry"), it unconditionally
overwrites vmcs12->vm_exit_instruction_len with exit_insn_len right after
vmcs12_save_pending_event() returns:

arch/x86/kvm/vmx/nested.c:prepare_vmcs12() {
    ...
		vmcs12_save_pending_event(vcpu, vmcs12,
					  vm_exit_reason, exit_intr_info);

		vmcs12->vm_exit_intr_info = exit_intr_info;
		vmcs12->vm_exit_instruction_len = exit_insn_len;
		vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
    ...
}

This also affects the existing kvm_exception_is_soft(nr) branch in
vmcs12_save_pending_event():

arch/x86/kvm/vmx/nested.c:vmcs12_save_pending_event() {
    ...
		if (kvm_exception_is_soft(nr)) {
			vmcs12->vm_exit_instruction_len =
				vcpu->arch.event_exit_inst_len;
			idt_vectoring |= INTR_TYPE_SOFT_EXCEPTION;
		} else
    ...
}

When a nested VM-Exit is synthesized while a software interrupt or software
exception is being vectored and vcpu->arch.event_exit_inst_len differs from
exit_insn_len (for instance, after vmx_cancel_injection() or after state
restore via KVM_SET_NESTED_STATE and KVM_SET_VCPU_EVENTS prior to
kvm_check_nested_events() -> nested_vmx_inject_exception_vmexit()), won't
prepare_vmcs12() clobber vmcs12->vm_exit_instruction_len with a stale or zero
exit_insn_len?

Should prepare_vmcs12() assign vmcs12->vm_exit_instruction_len = exit_insn_len
before calling vmcs12_save_pending_event() so that soft interrupts and soft
exceptions can override it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021042603.608292-1-ma.mandourr@gmail.com?part=1

[PATCH 2/2] KVM: selftests: Add test for nested VM-Exits while vectoring soft INTs

[ma.mandourr]

From: Mahmoud Abumandour <ma.mandourr@gmail.com>

Verify that a VM-Exit from L2 that occurs while vectoring INT3 or INTn
reports the instruction length in the VM-Exit instruction length field and
leaves L1's VM-Entry instruction length untouched.

Assisted-by: LLM
Signed-off-by: Mahmoud Abumandour <ma.mandourr@gmail.com>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../kvm/x86/vmx_soft_int_vectoring_test.c     | 105 ++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 888a7530a280..d5f8749413f2 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -140,6 +140,7 @@ TEST_GEN_PROGS_x86 += x86/vmx_exception_with_invalid_guest_state
 TEST_GEN_PROGS_x86 += x86/vmx_msrs_test
 TEST_GEN_PROGS_x86 += x86/vmx_invalid_nested_guest_state
 TEST_GEN_PROGS_x86 += x86/vmx_nested_la57_state_test
+TEST_GEN_PROGS_x86 += x86/vmx_soft_int_vectoring_test
 TEST_GEN_PROGS_x86 += x86/apic_bus_clock_test
 TEST_GEN_PROGS_x86 += x86/xapic_ipi_test
 TEST_GEN_PROGS_x86 += x86/xapic_state_test
diff --git a/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c b/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c
new file mode 100644
index 000000000000..e74510c49938
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Verify that a nested VM-Exit that occurs while L2 is vectoring a software
+ * exception (INT3) or a software interrupt (INTn) reports the length of the
+ * instruction in the VM-Exit instruction length field, and doesn't modify the
+ * VM-Entry instruction length field, a VM-Entry control that only L1 writes.
+ */
+#include "kvm_util.h"
+#include "processor.h"
+#include "vmx.h"
+#include "kselftest.h"
+
+#define INTR_INFO_VALID_MASK		BIT(31)
+#define INTR_TYPE_SOFT_INTR		(4 << 8)
+#define INTR_TYPE_SOFT_EXCEPTION	(6 << 8)
+
+#define SOFT_INT_VECTOR			0x40
+
+/*
+ * Arbitrary, but a legal instruction length that differs from the lengths of
+ * INT3 and INTn, i.e. a value no VM-Exit in this test can legitimately produce.
+ */
+#define ENTRY_INSN_LEN			7
+
+static void l2_guest_code(void)
+{
+	asm volatile("int3");
+	asm volatile("int $" __stringify(SOFT_INT_VECTOR));
+	vmcall();
+}
+
+static void l1_assert_vectoring_exit(const char *insn, u32 vectoring_info,
+				     u32 insn_len)
+{
+	u64 entry_insn_len;
+
+	GUEST_ASSERT_EQ(vmread(VM_EXIT_REASON), EXIT_REASON_EXCEPTION_NMI);
+	GUEST_ASSERT_EQ(vmread(VM_EXIT_INTR_INFO) & 0xff, GP_VECTOR);
+	GUEST_ASSERT_EQ(vmread(IDT_VECTORING_INFO_FIELD),
+			INTR_INFO_VALID_MASK | vectoring_info);
+	GUEST_ASSERT_EQ(vmread(VM_EXIT_INSTRUCTION_LEN), insn_len);
+
+	entry_insn_len = vmread(VM_ENTRY_INSTRUCTION_LEN);
+	__GUEST_ASSERT(entry_insn_len == ENTRY_INSN_LEN,
+		       "VM-Exit while vectoring %s changed the VM-Entry instruction length from %u to %lu",
+		       insn, ENTRY_INSN_LEN, entry_insn_len);
+
+	/* RIP points at the INT3/INTn, skip it. */
+	vmwrite(GUEST_RIP, vmread(GUEST_RIP) + insn_len);
+}
+
+static void l1_guest_code(struct vmx_pages *vmx_pages)
+{
+	prepare_for_vmx_operation(vmx_pages);
+	load_vmcs(vmx_pages);
+	prepare_vmcs(vmx_pages, l2_guest_code);
+
+	/*
+	 * Put every IDT entry beyond the limit so that vectoring any event in
+	 * L2 generates #GP, and intercept #GP so that the VM-Exit occurs while
+	 * the original event is being vectored.
+	 */
+	vmwrite(GUEST_IDTR_LIMIT, 0);
+	vmwrite(EXCEPTION_BITMAP, BIT(GP_VECTOR));
+	vmwrite(VM_ENTRY_INSTRUCTION_LEN, ENTRY_INSN_LEN);
+
+	vmlaunch();
+	l1_assert_vectoring_exit("INT3", INTR_TYPE_SOFT_EXCEPTION | BP_VECTOR, 1);
+
+	vmresume();
+	l1_assert_vectoring_exit("INTn", INTR_TYPE_SOFT_INTR | SOFT_INT_VECTOR, 2);
+
+	vmresume();
+	GUEST_ASSERT_EQ(vmread(VM_EXIT_REASON), EXIT_REASON_VMCALL);
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	gva_t vmx_pages_gva;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
+
+	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
+	vcpu_alloc_vmx(vm, &vmx_pages_gva);
+	vcpu_args_set(vcpu, 1, vmx_pages_gva);
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+	case UCALL_DONE:
+		break;
+	default:
+		TEST_FAIL("Unexpected ucall %lu", uc.cmd);
+	}
+
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.43.0

Re: [PATCH 2/2] KVM: selftests: Add test for nested VM-Exits while vectoring soft INTs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: standalone test duplicates nested_exceptions_test setup
--

commit 419533e2e5a5342a3cc80d072a4cecd9f800a093
Author: Mahmoud Abumandour <ma.mandourr@gmail.com>

KVM: selftests: Add test for nested VM-Exits while vectoring soft INTs

This commit adds a standalone KVM x86 selftest to verify that a nested
VM-Exit occurring while L2 vectors a software exception or interrupt reports
the instruction length in VM_EXIT_INSTRUCTION_LEN without modifying
VM_ENTRY_INSTRUCTION_LEN.

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 888a7530a2808..d5f8749413f27 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -140,6 +140,7 @@ TEST_GEN_PROGS_x86 += x86/vmx_exception_with_invalid_guest_state
>  TEST_GEN_PROGS_x86 += x86/vmx_msrs_test
>  TEST_GEN_PROGS_x86 += x86/vmx_invalid_nested_guest_state
>  TEST_GEN_PROGS_x86 += x86/vmx_nested_la57_state_test
> +TEST_GEN_PROGS_x86 += x86/vmx_soft_int_vectoring_test
>  TEST_GEN_PROGS_x86 += x86/apic_bus_clock_test
>  TEST_GEN_PROGS_x86 += x86/xapic_ipi_test
>  TEST_GEN_PROGS_x86 += x86/xapic_state_test
> diff --git a/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c b/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c
> new file mode 100644
> index 0000000000000..e74510c49938c
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c

[ ... ]

> +static void l1_guest_code(struct vmx_pages *vmx_pages)
> +{
> +	prepare_for_vmx_operation(vmx_pages);
> +	load_vmcs(vmx_pages);
> +	prepare_vmcs(vmx_pages, l2_guest_code);
> +
> +	/*
> +	 * Put every IDT entry beyond the limit so that vectoring any event in
> +	 * L2 generates #GP, and intercept #GP so that the VM-Exit occurs while
> +	 * the original event is being vectored.
> +	 */
> +	vmwrite(GUEST_IDTR_LIMIT, 0);
> +	vmwrite(EXCEPTION_BITMAP, BIT(GP_VECTOR));
> +	vmwrite(VM_ENTRY_INSTRUCTION_LEN, ENTRY_INSN_LEN);
> +
> +	vmlaunch();
> +	l1_assert_vectoring_exit("INT3", INTR_TYPE_SOFT_EXCEPTION | BP_VECTOR, 1);
> +
> +	vmresume();
> +	l1_assert_vectoring_exit("INTn", INTR_TYPE_SOFT_INTR | SOFT_INT_VECTOR, 2);
> +
> +	vmresume();
> +	GUEST_ASSERT_EQ(vmread(VM_EXIT_REASON), EXIT_REASON_VMCALL);
> +	GUEST_DONE();
> +}

[Severity: Medium]
This isn't a bug, but l1_guest_code() in
tools/testing/selftests/kvm/x86/vmx_soft_int_vectoring_test.c duplicates the
nested VMCS setup, empty GUEST_IDTR_LIMIT configuration, and GP_VECTOR
interception in EXCEPTION_BITMAP that l1_vmx_code() in
tools/testing/selftests/kvm/x86/nested_exceptions_test.c already sets up to
trigger nested VM-Exits during L2 event vectoring:

tools/testing/selftests/kvm/x86/nested_exceptions_test.c:l1_vmx_code() {
	prepare_for_vmx_operation(vmx);

	load_vmcs(vmx);

	prepare_vmcs(vmx, NULL);
	vmwrite(GUEST_IDTR_LIMIT, 0);
	...
	vmwrite(EXCEPTION_BITMAP, INTERCEPT_SS_GP_DF);
	...
}

Could the INT3 and INTn vectoring checks in l2_guest_code() and
l1_assert_vectoring_exit() be added to
tools/testing/selftests/kvm/x86/nested_exceptions_test.c instead of adding a
separate x86/vmx_soft_int_vectoring_test target to TEST_GEN_PROGS_x86 in
tools/testing/selftests/kvm/Makefile.kvm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021042603.608292-1-ma.mandourr@gmail.com?part=2