Re: [RFC PATCH 11/11] samples/kvm, selftests/kvm: Allow the gmem_provider NVMe DMA test on arm64

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] The gmem_provider sample module blindly trusts the `setup.kvm_fd` file descriptor, leading to type confusion and local privilege escalation.
- [High] A data race on the `info->kvm` pointer between `gmem_bind` and `gmem_fd_ioctl` allows a concurrent binding to drop the VM reference while it is actively being used, leading to a Use-After-Free.
- [Medium] The NVMe DMA selftest hardcodes the Amazon PCI Vendor ID (0x1d0f), causing it to fail on non-Amazon NVMe devices.
- [Medium] The selftest implicitly assumes a 4KB system page size, causing IOMMU mapping failures on architectures with larger page sizes.
- [Medium] The selftest bypasses architecture-specific I/O accessors and omits mandatory memory barriers, causing data races on weakly-ordered architectures.
- [Medium] The selftest performs raw, native-endian memory accesses on strictly little-endian PCI configuration space and NVMe registers, causing corruption on big-endian architectures.
--

--- Patch [11]: [RFC PATCH 11/11] samples/kvm, selftests/kvm: Allow the gmem_provider NVMe DMA test on arm64 ---
commit fc0ebbb277714921db32af720eb2c5783d061ea0
Author: David Woodhouse <dwmw@amazon.co.uk>

samples/kvm, selftests/kvm: Allow the gmem_provider NVMe DMA test on arm64

The sample provider and the NVMe DMA selftest have no x86 specifics.
Drop the X86_64 gate on the sample's Kconfig (arm64 also has
KVM_GUEST_MEMFD; the SEV RMP reset is already guarded by
CONFIG_AMD_MEM_ENCRYPT) and move the NVMe DMA selftest from
TEST_GEN_PROGS_x86 to TEST_GEN_PROGS_COMMON so it also builds on
architectures with an IOMMU (arm64 SMMU-v3, ...).

> diff --git a/samples/Kconfig b/samples/Kconfig
> index 3482659638b5b..d26a03dea0728 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -328,7 +328,7 @@ source "samples/damon/Kconfig"
>  
>  config SAMPLE_KVM_GMEM_PROVIDER
>  	tristate "Build sample guest_memfd provider -- loadable module only"
> -	depends on KVM_GUEST_MEMFD && X86_64 && m
> +	depends on KVM_GUEST_MEMFD && (X86_64 || ARM64) && m

[Severity: Critical]
Is it safe to blindly trust the kvm_fd here?

In gmem_ctl_ioctl(), the user-provided setup.kvm_fd is resolved and used
like this:

samples/kvm/gmem_provider.c:gmem_ctl_ioctl() {
    ...
	kvm_file = fget(setup.kvm_fd);
	if (!kvm_file)
		return -EBADF;
	kvm = kvm_file->private_data;
	if (!kvm) {
		fput(kvm_file);
		return -EINVAL;
	}
	kvm_get_kvm(kvm);
    ...
}

If a user passes an arbitrary file descriptor, it could lead to type confusion
since we don't verify the file's operations before extracting private_data and
performing an out-of-bounds refcount_inc(&kvm->users_count).

[Severity: High]
Can gmem_fd_ioctl() safely use info->kvm without synchronization?

In gmem_fd_ioctl(), info->kvm is used without holding a lock:

samples/kvm/gmem_provider.c:gmem_fd_ioctl() {
    ...
		if (info->kvm)
			kvm_gmem_invalidate_range(info->kvm,
				info->base_gfn + start_index - info->pgoff,
				info->base_gfn + end_index - info->pgoff);
    ...
}

Concurrently, gmem_bind() can swap info->kvm and drop the underlying
reference:

samples/kvm/gmem_provider.c:gmem_bind() {
    ...
	if (info->kvm != kvm) {
		kvm_get_kvm(kvm);
		if (info->kvm)
			kvm_put_kvm(info->kvm);
		info->kvm = kvm;
	}
    ...
}

Could this concurrent binding drop the VM reference to zero while it is being
accessed by the ioctl, leading to a use-after-free?

[ ... ]

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 7f46843e7428f..12004a487c326 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -58,6 +58,7 @@ TEST_PROGS_x86 += x86/nx_huge_pages_test.sh
>  # Compiled test targets valid on all architectures with libkvm support
>  TEST_GEN_PROGS_COMMON = demand_paging_test
>  TEST_GEN_PROGS_COMMON += dirty_log_test
> +TEST_GEN_PROGS_COMMON += gmem_provider_nvme_dma_test

[Severity: Medium]
Will this regression cause the test to fail on host hardware that doesn't use
an Amazon NVMe device?

The test hardcodes the Amazon PCI Vendor ID:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:main() {
    ...
		TEST_ASSERT(vid == 0x1d0f,
			    "unexpected VID 0x%04x in provider region -- Identify DMA landed elsewhere?\n"
    ...
}

Should the expected VID be read dynamically from offset 0x00 instead?

[Severity: Medium]
Does the implicit 4KB system page size assumption introduce a regression on
architectures that commonly use larger page sizes, such as arm64?

The test uses hardcoded 4KB offsets and lengths for IOMMU mapping:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:main() {
    ...
	TEST_ASSERT(!ioas_map_user(iommufd_fd, alloc.out_ioas_id, sq_buf, IOVA_SQ, 0x1000),
		    "IOAS_MAP(sq) errno=%d", errno);
	TEST_ASSERT(!ioas_map_user(iommufd_fd, alloc.out_ioas_id, cq_buf, IOVA_CQ, 0x1000),
		    "IOAS_MAP(cq) errno=%d", errno);
    ...
}

Since iommufd requires mapping lengths and IOVAs to be aligned to the system
page size, won't this fail on kernels running with 16KB or 64KB page sizes?

[Severity: Medium]
Can a data race occur on weakly-ordered architectures without memory barriers?

The custom MMIO accessors use plain volatile writes:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:wr32() {
    ...
static void     wr32(void *b, uint32_t o, uint32_t v) { *(volatile uint32_t *)((char *)b + o) = v; }
    ...
}

When ringing the doorbell:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:main() {
    ...
	/* Ring SQ0 tail doorbell. */
	wr32(bar, NVME_SQ0TDBL, 1);
    ...
}

Without an explicit memory barrier, is the CPU free to reorder the MMIO write
to the doorbell ahead of the normal RAM writes to the Submission Queue Entry?

[Severity: Medium]
Does this code corrupt device state on big-endian architectures?

Since the test is now built for all architectures, it will run on big-endian
systems. However, the test uses native-endian assignments to interact with
little-endian PCI configuration space and NVMe registers:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:main() {
    ...
	TEST_ASSERT(pread(vfio_fd, &pci_cmd, 2, cfg_off + 0x04) == 2, "cfg CMD read");
	pci_cmd |= 0x0006;
	TEST_ASSERT(pwrite(vfio_fd, &pci_cmd, 2, cfg_off + 0x04) == 2, "cfg CMD write");
    ...
}

And for the NVMe SQE:

tools/testing/selftests/kvm/gmem_provider_nvme_dma_test.c:main() {
    ...
		s[0]  = 0x00010006;	/* CID=1 << 16 | OPC=Identify */
    ...
}

Will these direct native-endian accesses produce byte-swapped data on
big-endian CPUs and cause the test to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f09647f427565c1789a6979470045aab78661563.1784194290.git.dwmw@amazon.co.uk?part=11