Re: [PATCH v2 28/28] 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/kvm: Race condition in pvclock test causes deadlock
--

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

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

This commit introduces memory attributes selftests to catch side-channel
vulnerabilities. It exercises various guest memory access patterns to verify
memory attribute access restrictions are upheld.

> diff --git a/tools/testing/selftests/kvm/memory_attributes.c b/tools/testing/selftests/kvm/memory_attributes.c
> --- a/tools/testing/selftests/kvm/memory_attributes.c
> +++ b/tools/testing/selftests/kvm/memory_attributes.c

[ ... ]

> @@ -86,6 +122,14 @@ static void guest_code(void *data)
>  			arch_controlled_exec(vaddr);
>  			GUEST_SYNC(stage++);
>  			break;
> +		case TEST_OP_MONITOR_ADDRESS: {
> +			if (arch_controlled_read(vaddr) != expected_val)
> +				GUEST_SYNC(stage++);
> +			break;
> +		}

[Severity: Medium]
Is there a risk of the guest loop getting stuck indefinitely?

In guest_code() under TEST_OP_MONITOR_ADDRESS, the guest spins and does not
trigger a VM-exit unless the value changes. If the host thread is blocked
from updating the expected value, this could cause a deadlock.

[ ... ]

> 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

[ ... ]

> +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;
> +
> +	pthread_create(&vcpu_thread, NULL, vcpu_worker, test_data);
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, paddr | 0x1);

[Severity: Medium]
Could this asynchronous sequence lead to a permanent deadlock?

The vcpu_worker thread executes KVM_RUN, which acquires the vcpu->mutex.
Concurrently, the main thread calls vcpu_set_msr(), which triggers
KVM_SET_MSRS and also attempts to acquire vcpu->mutex.

If the worker thread schedules and acquires vcpu->mutex before the main
thread executes vcpu_set_msr(), the main thread will block indefinitely.
Consequently, the MSR is never set, the guest memory is never updated,
and the guest spin loop under TEST_OP_MONITOR_ADDRESS never sees the
updated value. KVM_RUN will thus never return, hanging the test.

> +	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);
> +	pthread_create(&vcpu_thread, NULL, vcpu_worker, test_data);
> +	vcpu_set_msr(vcpu, MSR_KVM_SYSTEM_TIME_NEW, paddr | 0x1);

[Severity: Medium]
This appears to have the exact same race condition leading to deadlock
as the first thread handoff earlier in test_side_channel_pvclock().

If the vcpu_worker acquires vcpu->mutex before the main thread can execute
vcpu_set_msr(), the test will hang waiting for an MSR update that will never
arrive.

> +	usleep(msecs_to_usecs(1000));
> +	TEST_ASSERT_EQ(pthread_tryjoin_np(vcpu_thread, NULL), EBUSY);

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