Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Returning VM_FAULT_NOPAGE for invalid memory offsets without installing a PTE causes an unkillable infinite page fault loop. - [High] Per-CPU memory allocated for `run` and `reg_page` is leaked on driver module unload and upon CPU hotplug setup failures. - [High] Premature character device registration allows userspace to trigger page faults and ioctls before per-CPU data is initialized, bypassing the newly added `if (!page)` check and causing kernel panics or UAF. -- commit 33a60782bd60657bd70b537be47acdb4ffe78f38 Author: Hardik Garg <[email protected]> mshv_vtl: Check per-CPU register page before mmap Register-page setup is optional for each CPU. If allocation or registration fails, mshv_vtl_configure_reg_page() warns and leaves that CPU's reg_page NULL, but initial context setup continues successfully. Meanwhile, successful setup on another CPU sets the global mshv_has_reg_page flag. mshv_vtl_fault() checks this global flag before selecting the requested CPU's register page. With mixed setup results across online CPUs, the check passes even for a CPU with no register page, reaching get_page(NULL) when userspace faults in that mapping. Check the selected per-CPU register page before taking its reference and return VM_FAULT_SIGBUS if it is absent. Fixes: 7bfe3b8ea6e3 ("Drivers: hv: Introduce mshv_vtl driver") Cc: [email protected] > diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c > index 6e3c11c681717..d8225242c73f8 100644 > --- a/drivers/hv/mshv_vtl_main.c > +++ b/drivers/hv/mshv_vtl_main.c > @@ -819,6 +819,9 @@ static vm_fault_t mshv_vtl_fault(struct vm_fault *vmf) > return VM_FAULT_NOPAGE; > } [Severity: High] This isn't a bug introduced by this patch, but returning VM_FAULT_NOPAGE for an invalid memory offset without installing a PTE causes an unkillable infinite page fault loop. If a userspace process maps the /dev/mshv device and accesses an invalid page offset (neither MSHV_RUN_PAGE_OFFSET nor MSHV_REG_PAGE_OFFSET), this else branch is taken. Returning VM_FAULT_NOPAGE signals to the core MM subsystem that the fault handler successfully installed the page table entry, so the kernel returns to userspace without doing anything. Since no PTE was actually installed, the CPU will immediately re-fault on the same instruction, creating a 100% CPU hung task. Could we return VM_FAULT_SIGBUS here instead? > > + if (!page) > + return VM_FAULT_SIGBUS; [Severity: High] This is a pre-existing issue, but premature character device registration allows userspace to bypass this newly added check. In mshv_vtl_init(), the character device is exposed to userspace early: mshv_vtl_init() { ... ret = misc_register(&mshv_dev); ... } If a userspace process races the module initialization by opening /dev/mshv and mapping it before hv_vtl_setup_synic() allocates the per-CPU run page, mshv_vtl_cpu_run(cpu) will return NULL. When this happens, virt_to_page(NULL) is called: mshv_vtl_fault() { ... if (real_off == MSHV_RUN_PAGE_OFFSET) { page = virt_to_page(mshv_vtl_cpu_run(cpu)); ... } This produces a garbage non-NULL pointer, causing the new if (!page) check to pass, and the subsequent get_page() will dereference the garbage pointer and panic. If initialization subsequently fails, the module load aborts and .text is freed while the racing userspace process retains an open file descriptor, leading to a use-after-free. Could the device registration be deferred until after the per-CPU data is initialized? > + > get_page(page); > vmf->page = page; > [Severity: High] This is a pre-existing issue, but does this code leak the per-CPU pages allocated for run and reg_page on driver module unload and upon CPU hotplug setup failures? When setting up the hotplug state, the teardown callback is set to NULL: hv_vtl_setup_synic() { ... ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vtl:online", mshv_vtl_alloc_context, NULL); ... } When the module unloads, cpuhp_remove_state() is called, but without a teardown callback, the memory permanently allocated via __get_free_page() in mshv_vtl_alloc_context() is leaked: mshv_vtl_alloc_context() { ... per_cpu->run = (struct mshv_vtl_run *) __get_free_page(GFP_KERNEL | __GFP_ZERO); ... } Additionally, if the hotplug state setup fails midway, the successfully processed CPUs will not be cleaned up. Could a teardown callback be provided to free these pages? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
