Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Unvalidated physical memory mapping and integer overflow allow mapping arbitrary host RAM. - [Critical] Missing MMU notifier registration and TOCTOU race in MMIO fault handling. - [High] Architecture page size mismatch corrupts physical address mappings on ARM64. - [High] Hardcoded write permissions bypass host read-only restrictions for MMIO regions. - [Medium] Uninitialized stack variable `pfnmap_args`. --
Note: The format of this report is altered to provide a free-form summary of the findings, rather than using traditional inline diff quotes, to adhere to recitation restrictions. commit f5a60b27c18f019e0a418a3f28dcff85e1128ed4 Author: Mukesh R <[email protected]> Subject: mshv: Populate mmio mappings for PCI passthru This commit adds support for mapping mmio memory to the guest upon receiving an unmapped guest physical address intercept. It looks up the physical frame number from the driver and asks the hypervisor to establish the map. While reviewing this code, a few questions came up regarding the handling of the memory mappings. When the memory region is created from the user memory structure, it appears only the start address is validated against the virtual memory area. The page count is taken directly from user input. Later, in the intercept handler, a signed 32-bit integer is used to loop over the 64-bit page count. If a user provides an excessively large size, could the loop counter overflow to a negative value and prematurely terminate the validation loop? Would this bypass the ram checks and allow mapping arbitrary host memory into the guest, while also potentially causing a lockup without a scheduling point? Is there a race condition when fetching the physical frame number? The memory map read lock is dropped after resolving the address, and the hypercall is made without holding it. Since there does not seem to be a memory management unit interval notifier sequence check here, what happens if the host virtual memory area is unmapped or modified concurrently? Could the guest retain access to stale or reallocated host memory? Does the physical frame number calculation work correctly on architectures with varying page sizes? On arm64, the kernel page size might be 64KB, meaning the value returned by the pfnmap functions is based on 64KB units. However, the handler subtracts a 4KB-based offset from it. Will this result in an incorrect base physical address being passed to the hypervisor? Will this logic handle read-only device memory properly? The code unconditionally requests a write fault resolution and then hardcodes the writable flag for the hypervisor map call. If userspace maps a read-only memory region, will the fault fixup fail? If it does succeed, would the guest be granted write access to a read-only region? Does the pfnmap arguments structure need zero-initialization? The structure is declared on the stack and only the virtual memory area and address fields are explicitly populated. Could uninitialized padding or other structure members contain stack garbage that might affect the pfnmap operations? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
