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

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

[Magnus Kulke]

hv_call_deposit_pages() donates pages (deposit) to hypervisor for L2
guest on L1VH systems via HVCALL_DEPOSIT_MEMORY. The hypervisor takes
ownership of those pages and per contract revokes root partition
access to them, raising a #GP on access from the L1VH root partition.

However, the pages remain mapped in the kernel direct map, so kernel
code may still access them even though the hypervisor has revoked
access.

Helpers such as "load_unaligned_zeropad()" deliberately read past the
end of a buffer and across page boundaries. A read into an unmapped
page is tolerated and triggers a #PF, for which the kernel executes
a fixup in the exception table.

If such a call steps into a page that has been deposited, the access
raises a #GP by the hypervisor from which the above handler cannot
recover and the kernel panics:

  Oops: general protection fault, maybe for address 0xff1100941a3dfffc
  RIP: 0010:csum_partial+0xe5/0x110

This condition will appear on L1VH system that have created L2
partitions (and hence deposited pages) and exercise networking code
paths such as csum_partial() can trigger this condition when a buffer
ends close to a page boundary (e.g fffc in the above example).

The fix is to remove the deposited pages from the direct map before
they are passed to the hypervisor, and restore them when the hypervisor
returns them again.

We want to avoid flushing the TLB in the loop, so we use
set_direct_map_invalid_noflush() when marking a deposited page invalid
and flush the affected page ranges in one go ourselves. In the opposite
direction this is not required:

  > If a paging-structure entry is modified to change the P flag from
  > 0 to 1, no invalidation is necessary. This is because no TLB entry
  > or paging-structure cache entry is created with information from a
  > paging-structure entry in which the P flag is 0.

(Intel SDM Vol. 3, 4.10.4.3)

Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>

Changes since v4:

- Adjusted for set_direct_map_{default,invalid}_noflush() signature
  changes
- Stop direct-map removal at first failure, restore/free supplied
  pages within the helper
- Leak only pages that cannot be restored
- Extract contiguous pfn-run detection into helper

Changes since v3:

- Batch contiguous pages in the restore path
- Adjusted comment about failure modes

Changes since v2:
- Checkpatch format fix

Changes since RFC:
- Handle direct-map restoration failures without returning unmapped
  pages to the allocator.
- Move freeing of withdrawn pages into the restoration helper.
---
 drivers/hv/hv_proc.c           | 112 +++++++++++++++++++++++++++++++--
 drivers/hv/mshv_root_hv_call.c |   4 +-
 include/asm-generic/mshyperv.h |   4 ++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
index f811bb5c42a1..b63f8da1ecc1 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;
+}
+
 /*
  * Allocate free pages for deposit to hypervisor. pfna[] must be large enough
  * to hold HV_DEPOSIT_INP_MAX (511) pages. If num_pages is 512, return last
@@ -84,7 +137,7 @@ static int hv_alloc_dep_pages(int node, u64 *pfna, u64 *lastpfnp, int num_pages,
 static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
 {
 	struct hv_deposit_memory *hc_input;
-	int i, rc, num_pages;
+	int i, rc, num_pages, completed;
 	u64 status, *pfna, lastpfn = 0;
 	bool trunc_extra = false;
 
@@ -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;
+	}
+
+	if (lastpfn) {
+		rc = hv_remove_deposit_mapping(&lastpfn, 1);
+		if (rc) {
+			hv_restore_withdrawn_pages(pfna, num_pages);
+			goto out_free;
+		}
+	}
+
 	/* Not using hyperv_pcpu_input_arg, so no need to disable interrupts */
 
 	status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, num_pages, 0,
@@ -139,7 +207,7 @@ static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
 
 		if (!hv_result_success(status) && hv_repcomp(status) == 0)
 			/* We deposited lot earlier, so give it a go */
-			__free_page(pfn_to_page(lastpfn));
+			hv_restore_withdrawn_pages(&lastpfn, 1);
 	}
 
 	free_page((unsigned long)hc_input);
@@ -148,17 +216,51 @@ static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous)
 err_free_dep_pages:
 	hv_status_err(status, "\n");
 	rc = hv_result_to_errno(status);
+	completed = hv_repcomp(status);
 
-	for (i = hv_repcomp(status); i < num_pages; i++)
-		__free_page(pfn_to_page(pfna[i]));
+	hv_restore_withdrawn_pages(pfna + completed, num_pages - completed);
 	if (lastpfn)
-		__free_page(pfn_to_page(lastpfn));
+		hv_restore_withdrawn_pages(&lastpfn, 1);
 
 out_free:
 	free_page((unsigned long)hc_input);
 	return rc;
 }
 
+/*
+ * Put withdrawn pages back in the direct map and free them. Counterpart to
+ * the direct map removal done by hv_call_deposit_pages(). Leak the pages
+ * that failed to be restored.
+ */
+void hv_restore_withdrawn_pages(const u64 *pfns, int count)
+{
+	int i, j, end, n, ret;
+	struct page *page;
+
+	for (i = 0; i < count; i = end) {
+		page = pfn_to_page(pfns[i]);
+
+		end = find_consecutive_pfns(pfns, count, i);
+
+		n = end - i;
+		ret = set_direct_map_default_noflush(page, n);
+
+		for (j = i; j < end; ++j) {
+			page = pfn_to_page(pfns[j]);
+			/*
+			 * Retry individually on failed batch. We don't want
+			 * to return pages to the allocator that we couldn't mark
+			 * valid in the direct map.
+			 */
+			if (ret &&
+			    WARN_ON_ONCE(set_direct_map_default_noflush(page, 1)))
+				continue;
+			__free_page(page);
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(hv_restore_withdrawn_pages);
+
 int hv_deposit_memory_node(int node, u64 pt_id, u64 hv_status)
 {
 	int result = hv_result(hv_status);
diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
index b8d199f95299..80256ddaa768 100644
--- a/drivers/hv/mshv_root_hv_call.c
+++ b/drivers/hv/mshv_root_hv_call.c
@@ -44,7 +44,6 @@ int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
 	struct page *page;
 	u16 completed;
 	u64 status, withdrawn = 0;
-	int i;
 	unsigned long flags;
 
 	page = alloc_page(GFP_KERNEL);
@@ -67,8 +66,7 @@ int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
 
 		completed = hv_repcomp(status);
 
-		for (i = 0; i < completed; i++)
-			__free_page(pfn_to_page(output_page->gpa_page_list[i]));
+		hv_restore_withdrawn_pages(output_page->gpa_page_list, completed);
 
 		if (!hv_result_success(status)) {
 			if (hv_result(status) == HV_STATUS_NO_RESOURCES)
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 4c9f3353074a..b4dc9cbf0fa6 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -347,6 +347,7 @@ static inline bool hv_parent_partition(void)
 
 bool hv_result_needs_memory(u64 status);
 int hv_deposit_memory_node(int node, u64 partition_id, u64 status);
+void hv_restore_withdrawn_pages(const u64 *pfns, int count);
 int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id);
 int hv_call_notify_all_processors_started(void);
 bool hv_lp_exists(u32 lp_index);
@@ -361,6 +362,9 @@ static inline int hv_deposit_memory_node(int node, u64 partition_id, u64 status)
 {
 	return -EOPNOTSUPP;
 }
+
+static inline void hv_restore_withdrawn_pages(const u64 *pfns, int count) { }
+
 static inline int hv_call_add_logical_proc(int node, u32 lp_index, u32 acpi_id)
 {
 	return -EOPNOTSUPP;
-- 
2.34.1

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

[sashiko-bot]

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