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 <[email protected]> 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.d...@amazon.co.uk?part=11
