Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] Returning VM_FAULT_NOPAGE for unmapped/invalid offsets in mshv_vtl_fault creates an infinite page fault loop. - [High] A partial allocation failure of `reg_page` on a secondary CPU leads to a NULL pointer dereference upon mmap fault, due to global state mismatch. - [High] Registering the character device before CPU contexts are initialized exposes uninitialized memory or causes kernel panics. - [High] The module leaks an eventfd wait queue entry, leading to a guaranteed Use-After-Free (UAF) upon module unload. - [High] Missing allocation failure checks lead to a NULL pointer dereference in `mshv_vtl_hvcall_call`. - [Medium] The CPU bounds check in `mshv_vtl_ioctl_set_poll_file` uses `num_possible_cpus()` instead of `nr_cpu_ids`, potentially rejecting valid CPUs. -- --- Patch [1]: [PATCH] mshv: bounds-check cpu index in vtl mmap fault handler --- commit 8ac0cc752b2b67def52fafd91c16c26937bc5c16 Author: Yi Xie <[email protected]> mshv: bounds-check cpu index in vtl mmap fault handler cpu is taken from pgoff & 0xffff. cpu_online() does not reject cpu >= nr_cpu_ids, and per_cpu_ptr() can then walk off __per_cpu_offset. > diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c > index 0d3d4161974f8..fc50c44ac1bda 100644 > --- a/drivers/hv/mshv_vtl_main.c > +++ b/drivers/hv/mshv_vtl_main.c > @@ -801,7 +801,7 @@ static vm_fault_t mshv_vtl_fault(struct vm_fault *vmf) > int cpu = vmf->pgoff & MSHV_PG_OFF_CPU_MASK; > int real_off = vmf->pgoff >> MSHV_REAL_OFF_SHIFT; > > - if (!cpu_online(cpu)) > + if (cpu >= nr_cpu_ids || !cpu_online(cpu)) > return VM_FAULT_SIGBUS; [Severity: High] This is a pre-existing issue, but returning VM_FAULT_NOPAGE for unmapped or invalid offsets in mshv_vtl_fault() can create an infinite page fault loop: drivers/hv/mshv_vtl_main.c:mshv_vtl_fault() { ... if (real_off == MSHV_RUN_PAGE_OFFSET) { page = virt_to_page(mshv_vtl_cpu_run(cpu)); } else if (real_off == MSHV_REG_PAGE_OFFSET) { if (!mshv_has_reg_page) return VM_FAULT_SIGBUS; page = mshv_vtl_cpu_reg_page(cpu); } else { return VM_FAULT_NOPAGE; } ... Since VM_FAULT_NOPAGE indicates the fault was successfully handled without installing a PTE, won't the CPU infinitely re-fault on the same instruction and lock up the core if userspace accesses an unmapped offset? Should this return VM_FAULT_SIGBUS instead? [Severity: High] This is a pre-existing issue, but does a partial allocation failure on a secondary CPU lead to a NULL pointer dereference upon mmap fault? If alloc_page() fails in mshv_vtl_configure_reg_page() for a specific CPU, it returns without initializing the per-CPU reg_page: drivers/hv/mshv_vtl_main.c:mshv_vtl_configure_reg_page() { ... reg_page = alloc_page(GFP_KERNEL | __GFP_ZERO | __GFP_RETRY_MAYFAIL); if (!reg_page) { WARN(1, "failed to allocate register page\n"); return; } ... However, the fault handler relies on the global mshv_has_reg_page flag instead of checking if the per-CPU page is actually valid: drivers/hv/mshv_vtl_main.c:mshv_vtl_fault() { ... } else if (real_off == MSHV_REG_PAGE_OFFSET) { if (!mshv_has_reg_page) return VM_FAULT_SIGBUS; page = mshv_vtl_cpu_reg_page(cpu); } ... get_page(page); If the global flag is set by another CPU's successful allocation, won't mshv_vtl_fault() bypass the safety check and call get_page(NULL) for the failing CPU? [Severity: High] This is a pre-existing issue, but does registering the character device before CPU contexts are initialized expose uninitialized memory? drivers/hv/mshv_vtl_main.c:mshv_vtl_init() { ... ret = misc_register(&mshv_dev); if (ret) { ... ret = hv_vtl_setup_synic(); ... Since misc_register() exposes /dev/mshv to userspace before hv_vtl_setup_synic() allocates the per-CPU run and reg_page pointers, can a fast userspace process mmap the device and trigger a page fault resulting in virt_to_page(NULL) or get_page(NULL)? [Severity: Medium] This is a pre-existing issue, but does the bounds check in mshv_vtl_ioctl_set_poll_file() incorrectly reject valid CPUs on sparse topologies? drivers/hv/mshv_vtl_main.c:mshv_vtl_ioctl_set_poll_file() { ... if (input.cpu >= num_possible_cpus() || !cpu_online(input.cpu)) return -EINVAL; ... If the system has a sparse CPU possible mask, num_possible_cpus() could be lower than a valid CPU ID. Should this use nr_cpu_ids instead? [Severity: High] This is a pre-existing issue, but does the module leak an eventfd wait queue entry leading to a use-after-free upon module unload? drivers/hv/mshv_vtl_main.c:mshv_vtl_ioctl_set_poll_file() { ... if (file) { init_waitqueue_func_entry(&poll_file->wait, mshv_vtl_poll_file_wake); init_poll_funcptr(&poll_file->pt, mshv_vtl_ptable_queue_proc); vfs_poll(file, &poll_file->pt); } ... drivers/hv/mshv_vtl_main.c:mshv_vtl_exit() { ... device_del(mem_dev); kfree(mem_dev); misc_deregister(&mshv_vtl_low); ... The MSHV_SET_POLL_FILE ioctl permanently adds the global per-CPU poll_file->wait entry into the user-provided eventfd's wait queue. Since mshv_vtl_exit() doesn't remove the wait queue entry or release the eventfd reference, won't signaling the eventfd after module unload cause the kernel to traverse the wait queue and dereference freed memory? [Severity: High] This is a pre-existing issue, but are missing allocation failure checks causing a NULL pointer dereference in mshv_vtl_hvcall_call()? drivers/hv/mshv_vtl_main.c:mshv_vtl_hvcall_call() { ... in = (void *)__get_free_page(GFP_KERNEL); out = (void *)__get_free_page(GFP_KERNEL); if (copy_from_user(in, (void __user *)hvcall.input_ptr, hvcall.input_size)) { ... If __get_free_page() returns 0 (NULL) due to memory pressure, the subsequent copy_from_user() will attempt to copy user-supplied data to a NULL pointer. Shouldn't the return values of __get_free_page() be validated? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
