Re: [PATCH v5] mshv: remove deposited pages from direct map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mshv: unmapped direct-map access to deposited state and stats pages
- [Medium] mshv: hv_remove_deposit_mapping() restores untouched tail PFNs on error
--

commit 13157bed008befe9046572707b45f4f66588f031
Author: Magnus Kulke <magnuskulke@linux.microsoft.com>

mshv: remove deposited pages from direct map

This commit removes pages deposited to the hypervisor via
HVCALL_DEPOSIT_MEMORY from the kernel direct map before hypercall submission
and restores them to the direct map when withdrawn or when depositing fails.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index f811bb5c42a1d..b63f8da1ecc13 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
> @@ -7,12 +7,65 @@
>  #include <linux/cpuhotplug.h>
>  #include <linux/minmax.h>
>  #include <linux/export.h>
> +#include <linux/set_memory.h>
>  #include <asm/mshyperv.h>
> +#include <asm/tlbflush.h>
>  
>  #define HV_DEPOSIT_MAX 512
>  #define HV_DEPOSIT_INP_MAX ((HV_HYP_PAGE_SIZE -  \
>  	offsetof(struct hv_deposit_memory, gpa_page_list)) / sizeof(u64))
>  
> +static inline int find_consecutive_pfns(const u64 *pfns, int count, int start)
> +{
> +	int end;
> +
> +	if (start >= count)
> +		return count;
> +
> +	for (end = start + 1; end < count; ++end) {
> +		if (pfns[end] != pfns[end - 1] + 1)
> +			break;
> +	}
> +
> +	return end;
> +}
> +
> +/*
> + * Remove pages from the direct map before depositing them. On failure,
> + * restore the mapping and free all supplied pages, leaking pages whose
> + * mappings cannot be restored.
> + */
> +static int hv_remove_deposit_mapping(const u64 *pfns, int count)
> +{
> +	int i, end, ret;
> +	unsigned long addr, size;
> +
> +	for (i = 0; i < count; i = end) {
> +		end = find_consecutive_pfns(pfns, count, i);
> +
> +		ret = set_direct_map_invalid_noflush(pfn_to_page(pfns[i]),
> +						   end - i);
> +		if (ret)
> +			goto restore;
> +	}
> +
> +	for (i = 0; i < count; i = end) {
> +		addr = (unsigned long)page_address(pfn_to_page(pfns[i]));
> +
> +		end = find_consecutive_pfns(pfns, count, i);
> +
> +		size = (unsigned long)(end - i) << PAGE_SHIFT;
> +		flush_tlb_kernel_range(addr, addr + size);
> +	}
> +
> +	return 0;
> +
> +restore:
> +	/* The failing run may have been partially modified too. */
> +	hv_restore_withdrawn_pages(pfns, count);
> +	return ret;
> +}

[Severity: Medium]
When set_direct_map_invalid_noflush() fails on run [i, end) and jumps to
restore, the trailing PFN runs in [end, count) have not been touched and
still have valid default mappings in the direct map.

Passing the full count to hv_restore_withdrawn_pages(pfns, count) runs
set_direct_map_default_noflush() on those untouched pages in [end, count) as
well:

drivers/hv/hv_proc.c:hv_restore_withdrawn_pages() {
    ...
		ret = set_direct_map_default_noflush(page, n);

		for (j = i; j < end; ++j) {
			page = pfn_to_page(pfns[j]);
			if (ret &&
			    WARN_ON_ONCE(set_direct_map_default_noflush(page, 1)))
				continue;
			__free_page(page);
		}
}

On arm64, set_direct_map_default_noflush() calls update_range_prot(), which
unconditionally calls split_kernel_leaf_mapping() before updating PTEs:

arch/arm64/mm/pageattr.c:update_range_prot() {
    ...
	ret = split_kernel_leaf_mapping(start, start + size);
	if (WARN_ON_ONCE(ret))
		return ret;
    ...
}

If set_direct_map_invalid_noflush() failed with -ENOMEM while splitting a
large leaf mapping, wouldn't set_direct_map_default_noflush() also fail to
split leaf mappings for the untouched pages in [end, count), triggering
WARN_ON_ONCE() and leaking pages in pfns[end .. count - 1] that were never
removed from the direct map?

Should the restore path only call hv_restore_withdrawn_pages(pfns, end) and
free the untouched pages in [end, count) directly via __free_page()?

[ ... ]

> @@ -125,6 +178,21 @@ static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
>  		num_pages = HV_MAX_CONTIGUOUS_ALLOCATION_PAGES;
>  	}
>  
> +	rc = hv_remove_deposit_mapping(pfna, num_pages);
> +	if (rc) {
> +		if (lastpfn)
> +			__free_page(pfn_to_page(lastpfn));
> +		goto out_free;
> +	}

[Severity: High]
Does unconditionally removing all deposited pages from the direct map cause
an unhandled page fault when the hypervisor maps VP state pages or stats
pages out of the deposited memory pool?

hv_deposit_memory_node() -> hv_call_deposit_pages() ->
hv_remove_deposit_mapping() marks all deposited pages invalid in the direct
map on both bare-metal root partitions and L1VH partitions.

When mshv_use_overlay_gpfn() is false (such as on bare-metal root partitions
where hv_l1vh_partition() is false, or on L1VH partitions without
vmm_can_provide_overlay_gpfn), hv_map_vp_state_page() and
hv_map_stats_page() do not supply a caller-allocated page. Instead,
HVCALL_MAP_VP_STATE_PAGE and HVCALL_MAP_STATS_PAGE allocate a page from the
partition's deposited memory pool and return its PFN in output->map_location:

drivers/hv/mshv_root_hv_call.c:hv_call_map_vp_state_page() {
    ...
		if (!hv_result_needs_memory(status)) {
			if (hv_result_success(status))
				*state_page = pfn_to_page(output->map_location);
			local_irq_restore(flags);
			ret = hv_result_to_errno(status);
			break;
		}

		local_irq_restore(flags);

		ret = hv_deposit_memory(partition_id, status);
    ...
}

drivers/hv/mshv_root_hv_call.c:hv_call_map_stats_page() {
    ...
		ret = hv_deposit_memory(hv_current_partition_id, status);
		if (ret)
			return ret;
	} while (!ret);

	*addr = page_address(pfn_to_page(pfn));
    ...
}

Those returned PFNs are then converted to direct-map virtual addresses via
page_to_virt() in mshv_partition_ioctl_create_vp() (for
vp->vp_intercept_msg_page, vp->vp_register_page, and vp->vp_ghcb_page) and
page_address() in hv_call_map_stats_page() (as well as memremap(...,
MEMREMAP_WB) for root-partition SynIC pages when secondary CPUs deposit
memory to hv_current_partition_id) without restoring their direct-map PTEs.

When the kernel subsequently dereferences these addresses, for example in
mshv_debugfs_hv_stats_create() reading
stats[HV_HYPERVISOR_COUNTER_LOGICAL_PROCESSORS] or during VP execution in
mshv_vp_ioctl_run_vp() and mshv_vp_handle_intercept(), won't it fault on a
non-present direct-map page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103152416.404545-1-magnuskulke@linux.microsoft.com?part=1