Re: [PATCH] mshv: bounds-check cpu index in vtl mmap fault handler

[sashiko-bot@kernel.org]

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