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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] `test_side_channel_pvclock` assumes KVM periodically updates `tsc_timestamp` in the background, which is false and causes the guest to hang indefinitely.
- [Medium] `test_side_channel_hyperv_hypercall_inputs` fails to zero-initialize the `struct hv_send_ipi_ex` object in reused guest memory, causing the hypercall to fail.
- [Low] `test_side_channel_hyperv_hypercall_inputs` computes the physical address using stale `test_data->vaddr` instead of the passed parameter.
- [Low] `vcpu_translate()` passes a partially uninitialized `struct kvm_translation` to the KVM ioctl.
--

--- Patch [24]: [PATCH 24/24] KVM: x86: selftests: Introduce memory attributes side-channel tests ---
commit 4f5cb036a6a68d87d1e673199973b66af14cd6a4
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: x86: selftests: Introduce memory attributes side-channel tests
    
Introduce memory attributes selftests to catch any vulnerable
side-channels.

> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 54d73ea46e11f..3160a25966a10 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -883,6 +883,16 @@ static inline int vcpu_get_stats_fd(struct kvm_vcpu *vcpu)
>  	return fd;
>  }
>  
> +static inline struct kvm_translation vcpu_translate(struct kvm_vcpu *vcpu,
> +						    u64 gva)
> +{
> +	struct kvm_translation tr;
> +
> +	tr.linear_address = gva;
> +	vcpu_ioctl(vcpu, KVM_TRANSLATE, &tr);

[Severity: Low]
Does this pass uninitialized stack memory to the kernel?

Since only linear_address is initialized, the remainder of the struct contains
uninitialized stack memory. When the kernel reads the struct via
copy_from_user(), it will read this uninitialized memory, which can trigger
MemorySanitizer or Valgrind failures in CI environments.

Should this struct be zero-initialized first?

> +	return tr;
> +}
> +
>  int __kvm_has_device_attr(int dev_fd, u32 group, u64 attr);

[ ... ]

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

[ ... ]

> @@ -88,6 +125,25 @@ static void guest_code(void *data)
>  				     :: "b" (vaddr): "memory");
>  			GUEST_SYNC(stage++);
>  			break;
> +		case TEST_OP_HYPERV_HYPERCALL_INPUT: {
> +			hyperv_hypercall(HVCALL_SEND_IPI_EX, paddr, 0);
> +			asm volatile ("sti; hlt; cli;");
> +			GUEST_ASSERT_EQ(ipis_rcvd, 1);
> +			GUEST_SYNC(stage++);
> +			break;
> +		}
> +		case TEST_OP_MONITOR_ADDRESS: {
> +			uint64_t *pval = (uint64_t *)vaddr;
> +			uint64_t val = READ_ONCE(*pval);
> +
> +			while (READ_ONCE(*pval) == val &&
> +			       /* So host can force the op out of the loop */
> +			       READ_ONCE(test_data->op) == TEST_OP_MONITOR_ADDRESS)
> +				asm volatile("nop");

[Severity: Medium]
Can this loop hang the guest indefinitely?

In test_side_channel_pvclock(), the guest is expected to exit this loop when
the pvclock tsc_timestamp changes. However, KVM only updates the pvclock
structure synchronously before entering the guest (e.g., when processing
KVM_REQ_CLOCK_UPDATE).

Since the guest is spinning here without causing a VM-exit, KVM never gets a
chance to process clock updates, tsc_timestamp will never change, and the host
thread will time out waiting in pthread_tryjoin_np().

> +
> +			GUEST_SYNC(stage++);
> +			break;
> +		}
>  #endif

[ ... ]

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

[ ... ]

> +static void test_side_channel_hyperv_hypercall_inputs(struct kvm_vcpu *vcpu,
> +						      gva_t vaddr,
> +						      size_t size)
> +{
> +	struct kvm_vm *vm = vcpu->vm;
> +	struct hv_send_ipi_ex *ipi_ex = addr_gva2hva(vm, vaddr);
> +	gpa_t paddr = addr_gva2gpa(vm, test_data->vaddr);

[Severity: Low]
Does this use stale global state instead of the vaddr parameter?

Using test_data->vaddr here before it gets updated for this test phase relies
on it holding a left-over value from the previous test run. Should this use the
vaddr parameter passed into the function instead?

> +
> +	if (!kvm_has_cap(KVM_CAP_HYPERV_SEND_IPI) ||
> +	    !kvm_has_cap(KVM_CAP_HYPERV_HCALL_FAULT_EXIT))
> +		return;
> +
> +	vm_enable_cap(vcpu->vm, KVM_CAP_HYPERV_HCALL_FAULT_EXIT, 1);
> +
> +	ipis_rcvd = 0;
> +	vm_install_exception_handler(vm, IPI_VECTOR, guest_ipi_handler_hv);
> +	vcpu_set_msr(vcpu, HV_X64_MSR_GUEST_OS_ID, HYPERV_LINUX_OS_ID);
> +
> +	ipi_ex->vector = IPI_VECTOR;
> +	ipi_ex->vp_set.format = HV_GENERIC_SET_ALL;

[Severity: Medium]
Will this hypercall fail due to an uninitialized reserved field?

The struct hv_send_ipi_ex object is mapped over previously used guest memory
(vaddr). Because the reserved field isn't initialized here, it will contain
garbage data left over from prior tests.

The KVM handler for HVCALL_SEND_IPI_EX explicitly checks that reserved == 0 and
will return HV_STATUS_INVALID_PARAMETER otherwise, causing the test to fail.

Should the struct be cleared using memset() first?

> +	vm_set_memory_attributes(vm, paddr, vm->page_size, KVM_MEMORY_ATTRIBUTE_NO_ACCESS);
> +	test_page_restricted(vcpu, TEST_OP_HYPERV_HYPERCALL_INPUT, vaddr, paddr,
> +			     KVM_MEMORY_EXIT_FLAG_READ);
> +	vm_set_memory_attributes(vm, paddr, vm->page_size, 0);

[ ... ]

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