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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kselftests: race condition in pvclock test using usleep() for synchronization
--

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

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

This commit introduces memory attributes selftests to catch vulnerable
side-channels. It validates that KVM operations requiring memory access
properly account for memory attributes across various use cases.

> diff --git a/tools/testing/selftests/kvm/x86/memory_attributes.c b/tools/testing/selftests/kvm/x86/memory_attributes.c
> --- a/tools/testing/selftests/kvm/x86/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/x86/memory_attributes.c
[ ... ]
> @@ -181,3 +192,224 @@ static void arch_test_memory_access_pte(struct kvm_vcpu *vcpu, gva_t vaddr)
[ ... ]
> +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);

[Severity: Medium]
Does this test rely on a hardcoded 1-second timeout for synchronization?

On heavily loaded CI systems or nested virtualized environments, vCPU
scheduling and the pvclock update can easily exceed 1 second. This could
cause pthread_tryjoin_np() to fail unexpectedly and create a false positive
regression.

Similarly, when waiting to confirm the vCPU does not exit in the second part
of this test, a slow system might simply not have scheduled the thread yet,
leading to a false negative pass even if memory was erroneously updated.

> +	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);
> +	pthread_create(&vcpu_thread, NULL, vcpu_worker, test_data);
> +	usleep(msecs_to_usecs(1000));
> +	TEST_ASSERT_EQ(pthread_tryjoin_np(vcpu_thread, NULL), EBUSY);
> +
> +	/* Force the 'monitor_address' guest operation to finish */
> +	test_data->op = TEST_OP_NOP;
> +	TEST_ASSERT_EQ(pthread_join(vcpu_thread, NULL), 0);

[Severity: Medium]
Is a WRITE_ONCE() missing when assigning test_data->op?

The test_data->op variable is polled concurrently by the guest vCPU thread
via READ_ONCE() in guest_code(), so using a plain assignment here violates
standard lockless concurrency patterns.

> +	vm_set_memory_attributes(vm, paddr, vm->page_size, 0);
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918135030.171564-2-pbonzini@redhat.com?part=31