Re: [PATCH 44/45] KVM: x86: selftests: Introduce memory attributes side-channel tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: selftests: missing vcpu_force_vapic_update() in vapic side-channel test
- [Medium] KVM: selftests: pvclock side-channel test bypasses active gpc invalidation
- [Medium] KVM: selftests: unconditional vcpu_set_hv_cpuid() in main() breaks !HYPERV
--

commit d4f11e28c38cae048fc4237383806de4018441e7
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86: selftests: Introduce memory attributes side-channel tests

This commit introduces x86 selftests to verify that guest-initiated KVM
operations accessing guest memory via kvm_read/write_guest(),
gfn_to_hva_cache, gfn_to_pfn_cache, and the guest page walker respect memory
attribute restrictions.

Link: https://patch.msgid.link/20260918135030.171564-32-pbonzini@redhat.com

> diff --git a/tools/testing/selftests/kvm/memory_attributes.c b/tools/testing/selftests/kvm/memory_attributes.c
> index c5305b4f3dbe7..2883f6c772bee 100644
> --- a/tools/testing/selftests/kvm/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/memory_attributes.c

[ ... ]

> @@ -362,11 +407,15 @@ int main(int argc, char *argv[])
>  	test_mem = vm_alloc_pages(vm, pages);
>  	virt_map(vcpu->vm, MMIO_GVA, MMIO_GPA, 1);
>  	test_data = init_test_data(vcpu);
> +#ifdef __x86_64__
> +	vcpu_set_hv_cpuid(vcpu);
> +#endif

[Severity: Medium]
What happens on an x86_64 kernel built with CONFIG_KVM_HYPERV=n?

In test_side_channel_hyperv_hypercall_inputs(), the test checks
kvm_has_cap(KVM_CAP_HYPERV_SEND_IPI) so it can skip the Hyper-V subtest when
Hyper-V support is unavailable.

However, main() unconditionally calls vcpu_set_hv_cpuid(vcpu) ->
kvm_get_supported_hv_cpuid(), which invokes
kvm_ioctl(kvm_fd, KVM_GET_SUPPORTED_HV_CPUID, cpuid) and asserts success.
Because KVM_GET_SUPPORTED_HV_CPUID returns -EINVAL when CONFIG_KVM_HYPERV is
disabled, will this abort the entire test binary in main() before any tests
can run unless vcpu_set_hv_cpuid(vcpu) is guarded by a capability check such
as kvm_has_cap(KVM_CAP_SYS_HYPERV_CPUID) or
kvm_has_cap(KVM_CAP_HYPERV_SEND_IPI)?

> diff --git a/tools/testing/selftests/kvm/x86/memory_attributes.c b/tools/testing/selftests/kvm/x86/memory_attributes.c
> index 12395feb6ac7a..36e788be0e05d 100644
> --- a/tools/testing/selftests/kvm/x86/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/x86/memory_attributes.c

[ ... ]

> +/*
> + * Setup the vapic address on a GPA that is write-protected. Force an vapic
> + * update and validate its contents were not changes. Then, lift the write
> + * restriction and validate the page's contents are updated.
> + */
> +static void test_side_channel_vapic_addr(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vm *vm = vcpu->vm;
> +	gva_t vaddr = vm_alloc_page(vm);
> +	gpa_t paddr = addr_gva2gpa(vm, vaddr);
> +
> +	vm_set_vapic_addr(vcpu, paddr);
> +	test_data->op = TEST_OP_READ;
> +	test_data->vaddr = vaddr;
> +	test_data->confirm_read = 1;
> +	test_data->expected_val = ~0ULL >> 32;
> +	memset(addr_gva2hva(vm, vaddr), 0xff, sizeof(uint32_t));
> +	vm_set_memory_attributes(vm, paddr, vm->page_size, KVM_MEMORY_ATTRIBUTE_NW);
> +	vcpu_run_and_inc_stage(vcpu);
> +
> +	vm_set_memory_attributes(vm, paddr, vm->page_size, 0);
> +	vcpu_force_vapic_update(vcpu);
> +	test_data->expected_val = 0ULL;
> +	vcpu_run_and_inc_stage(vcpu);

[Severity: Medium]
Should vcpu_force_vapic_update(vcpu) also be called before the first
vcpu_run_and_inc_stage(vcpu) while KVM_MEMORY_ATTRIBUTE_NW is set?

In arch/x86/kvm/x86.c, vcpu_enter_guest() only calls
kvm_lapic_sync_to_vapic(vcpu) when KVM_REQ_EVENT (or an interrupt window) is
pending:

	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win ||
	    kvm_xen_has_interrupt(vcpu)) {
		++vcpu->stat.req_event;
		...
		if (kvm_lapic_enabled(vcpu)) {
			kvm_lapic_update_cr8_intercept(vcpu);
			kvm_lapic_sync_to_vapic(vcpu);
		}
	}

Neither vm_set_vapic_addr() (KVM_SET_VAPIC_ADDR) nor
vm_set_memory_attributes() (KVM_SET_MEMORY_ATTRIBUTES) sets KVM_REQ_EVENT on
the vCPU. Without calling vcpu_force_vapic_update(vcpu) before the first
vcpu_run_and_inc_stage(vcpu), won't kvm_lapic_sync_to_vapic() be skipped on
that entry, allowing the write-protected check to pass even if
kvm_write_guest_cached() did not honor memory attributes?

[ ... ]

> +static void test_side_channel_pvclock(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vm *vm = vcpu->vm;
> +	gva_t vaddr = vm_alloc_page(vm);
> +	gpa_t paddr = addr_gva2gpa(vm, vaddr);
> +	struct pvclock_vcpu_time_info *pvclock = addr_gpa2hva(vm, paddr);
> +
> +	pvclock->version = 0;
> +
> +	test_data->op = TEST_OP_MONITOR_ADDRESS;
> +	test_data->vaddr = vaddr + offsetof(struct pvclock_vcpu_time_info, version);
> +	test_data->expected_val = pvclock->version;
> +
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, paddr | 0x1);
> +	pthread_create(&vcpu_thread, NULL, vcpu_worker, test_data);
> +	usleep(msecs_to_usecs(1000));
> +	TEST_ASSERT_EQ(pthread_tryjoin_np(vcpu_thread, NULL), 0);
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, 0);
> +
> +	test_data->expected_val = pvclock->version;
> +	vm_set_memory_attributes(vm, paddr, vm->page_size, KVM_MEMORY_ATTRIBUTE_NW);
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, paddr | 0x1);

[Severity: Medium]
Does deactivating MSR_KVM_SYSTEM_TIME_NEW before setting
KVM_MEMORY_ATTRIBUTE_NW prevent this test from exercising invalidation of an
already-active gfn_to_pfn_cache?

Writing 0 to MSR_KVM_SYSTEM_TIME_NEW calls
kvm_gpc_deactivate(&vcpu->arch.pv_time), which clears gpc->valid and
gpc->active before vm_set_memory_attributes() updates the attributes. When
paddr | 0x1 is subsequently written to MSR_KVM_SYSTEM_TIME_NEW,
kvm_gpc_activate() -> __kvm_gpc_refresh() runs synchronously during
KVM_SET_MSRS, sees KVM_MEMORY_ATTRIBUTE_NW already in place, updates
gpc->attrs_generation to the current generation, and marks gpc->valid = false.

As a result, when vcpu_worker enters the guest and kvm_setup_guest_pvclock()
calls kvm_gpc_check(), the cache is already invalid and its attrs_generation
already matches the VM's generation. Would keeping the cache active across
vm_set_memory_attributes() (or writing paddr | 0x1 to MSR_KVM_SYSTEM_TIME_NEW
to queue KVM_REQ_GLOBAL_CLOCK_UPDATE before calling
vm_set_memory_attributes() with KVM_MEMORY_ATTRIBUTE_NW) better test that
kvm_gpc_check() detects the generation mismatch on an active, valid cache?

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