Re: [PATCH 23/24] KVM: x86: selftests: Introduce memory attributes PTE test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `test_memory_access_sync_spte()` incorrectly passes a Guest Virtual Address (`vaddr`) to `arch_write_return_insn()`, which requires a Guest Physical Address (`paddr`), causing test crashes or memory corruption.
- [Medium] `test_page()` silently ignores guest-side assertions (`UCALL_ABORT`), potentially masking test failures and leaving the host/guest state out of sync.
--

--- Patch [23]: [PATCH 23/24] KVM: x86: selftests: Introduce memory attributes PTE test ---
commit 129595776ab1f8d4ce03183d898471949d9d7971
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

    KVM: x86: selftests: Introduce memory attributes PTE test

This patch introduces tests to verify the behavior of memory attributes
(non-accessible and read-only) when installed on a page that holds guest
page table entries.

> diff --git a/tools/testing/selftests/kvm/memory_attributes.c b/tools/testing/selftests/kvm/memory_attributes.c
> index 7066ae791b99a..c66d5d085a9c1 100644
> --- a/tools/testing/selftests/kvm/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/memory_attributes.c

[ ... ]

> @@ -106,6 +121,21 @@ static void vcpu_run_and_inc_stage(struct kvm_vcpu *vcpu)
>  	test_data->stage++;
>  }
>  
> +static int test_page(struct kvm_vcpu *vcpu, int op, gva_t vaddr)
> +{
> +	int rc;
> +
> +	test_data->op = op;
> +	test_data->vaddr = vaddr;
> +
> +	rc = _vcpu_run(vcpu);
> +
> +	if (rc >= 0)
> +		test_data->stage++;

[Severity: Medium]
If the guest asserts (e.g., via GUEST_ASSERT), will this silently ignore the
failure?

When a guest assertion fires, KVM exits with KVM_EXIT_IO and the guest spins
in UCALL_ABORT. _vcpu_run() returns 0 in this case. Without decoding the
ucall struct, the test assumes the operation succeeded, increments the stage,
and continues, which could mask test failures.

> +
> +	return rc < 0 ? -errno : rc;
> +}

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/memory_attributes.c b/tools/testing/selftests/kvm/x86/memory_attributes.c
> index 2e1148f5146dd..3d2bcfe718848 100644
> --- a/tools/testing/selftests/kvm/x86/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/x86/memory_attributes.c

[ ... ]

> +static void test_memory_access_sync_spte(struct kvm_vcpu *vcpu, gva_t vaddr)
> +{
> +	struct kvm_vm *vm = vcpu->vm;
> +	gpa_t paddr = addr_gva2gpa(vm, vaddr);
> +	uint64_t *pte, old_pte, new_pte;

[ ... ]

> +	/* The not executable attrs remain valid */
> +	arch_write_return_insn(vm, vaddr);

[Severity: Medium]
Does this incorrectly pass a Guest Virtual Address (vaddr) to a function that
expects a Guest Physical Address?

arch_write_return_insn() takes a gpa_t and passes it to addr_gpa2hva():

void arch_write_return_insn(struct kvm_vm *vm, gpa_t vaddr)
{
	memset(addr_gpa2hva(vm, vaddr), 0xc3, 1);
}

If fed a GVA instead of the locally calculated paddr, will this cause the test
to crash due to a failed memory mapping translation, or silently corrupt the
wrong host virtual address?

> +	test_page_restricted(vcpu, TEST_OP_EXEC, vaddr, paddr,
> +			     KVM_MEMORY_EXIT_FLAG_EXEC);
> +
> +	/* Cleanup */
> +	*pte = old_pte;
> +	vm_set_memory_attributes(vm, paddr, vm->page_size, 0);
> +	test_page_accessible(vcpu, TEST_OP_EXEC, vaddr);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716181456.402786-1-pbonzini@redhat.com?part=23