Re: [PATCH v2 8/8] KVM: selftests: Add nested SVM DecodeAssists test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unmapping the page containing `npf_target` may accidentally unmap other variables like `l2_guest_stack`, causing unintended page faults.
- [Medium] Guest assertion failures are obscured because `get_ucall()`'s return value is checked with a simple equality assertion rather than handling `UCALL_ABORT`.
--

--- Patch [8]: [PATCH v2 8/8] KVM: selftests: Add nested SVM DecodeAssists test ---
commit 022271cf32a29cba4b8af8cdce602deac0fd3ec4
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: selftests: Add nested SVM DecodeAssists test

This commit adds a nested SVM selftest for DecodeAssists to verify that KVM
exposes the feature to L1 and provides the correct architectural exit state
for various intercepts.

> diff --git a/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c b/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c
> new file mode 100644
> index 0000000000000..ac9601dda9365
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/svm_nested_decode_assists_test.c
> @@ -0,0 +1,694 @@

[ ... ]

> +#define USERSPACE_PF_STAGE 1
> +#define TEST_IOPM_SIZE (3 * PAGE_SIZE)
> +
> +static u64 npf_target __aligned(PAGE_SIZE);
> +static u8 boundary_outsb_code[2 * PAGE_SIZE] __aligned(PAGE_SIZE);
> +static u8 limit_outsb_code[PAGE_SIZE] __aligned(PAGE_SIZE);
> +static unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];

[ ... ]

> +	tdp_identity_map_default_memslots(vm);
> +	pte = tdp_get_pte(vm, npf_gpa);
> +	*pte &= ~PTE_PRESENT_MASK(&vm->stage2_mmu);

[Severity: Medium]
Since npf_target is only 8 bytes long but page-aligned, could the linker
pack other variables (like l2_guest_stack) into the remaining 4088 bytes
of the same page?

If so, when this code unmaps the stage-2 PTE for the page containing
npf_target, it might accidentally unmap the guest stack. Would L2
execution then #NPF on its first stack operation and fail the test
misleadingly?

> +	pte = tdp_get_pte(vm, fetch_npf_gpa);
> +	*pte &= ~PTE_PRESENT_MASK(&vm->stage2_mmu);
> +
> +	boundary_page_gva = (gva_t)&boundary_outsb_code[PAGE_SIZE];
> +	pte = vm_get_pte(vm, boundary_page_gva);
> +	*pte &= ~PTE_PRESENT_MASK(&vm->mmu);
> +
> +	vcpu_args_set(vcpu, 5, svm_gva, npf_gpa, fetch_npf_gva,
> +		      fetch_npf_gpa, iopm_gpa);
> +	vcpu_run(vcpu);
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> +	{
> +		struct ucall uc;
> +
> +		TEST_ASSERT_EQ(get_ucall(vcpu, &uc), UCALL_SYNC);

[Severity: Medium]
If a guest assertion fails, it exits with UCALL_ABORT containing detailed
file, line, and message information.

By using a simple equality assertion here, wouldn't a guest failure result
in a generic host-side failure (e.g., "Expected UCALL_SYNC, got UCALL_ABORT")
and drop the actual guest failure reason, making it harder to debug?

> +		TEST_ASSERT_EQ(uc.args[1], USERSPACE_PF_STAGE);
> +	}
> +	queue_userspace_pf(vcpu);
> +	vcpu_run(vcpu);
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> +	TEST_ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);

[Severity: Medium]
Similarly, wouldn't this assertion also silently drop the payload of a
UCALL_ABORT if the guest aborts at the end of the test?

> +
> +	kvm_vm_free(vm);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783999988.git.zhang_wei@open-hieco.net?part=8