Re: [PATCH] mshv_vtl: Check per-CPU register page before mmap

[Hardik Garg <hargar@linux.microsoft.com>]

On 9/11/2026 11:05 AM, sashiko-bot@kernel.org wrote:
> 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.
> --

All the reported issues are pre-existing and not related to this patch.
I will review the issues and fix them in a separate patch.

> commit 33a60782bd60657bd70b537be47acdb4ffe78f38
> Author: Hardik Garg <hargar@linux.microsoft.com>
>
> 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: stable@vger.kernel.org
>
>> 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?
>