* Re: [PATCH] mshv: bounds-check cpu index in vtl mmap fault handler
2026-07-09 2:19 [PATCH] mshv: bounds-check cpu index in vtl mmap fault handler Yi Xie
@ 2026-07-09 2:51 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-09 2:51 UTC (permalink / raw)
To: Yi Xie; +Cc: linux-hyperv
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 <xieyi@kylinos.cn>
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/20260709021947.49436-1-xieyi@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread