[RFC PATCH] drivers/hv: remove deposited pages from direct map

[RFC PATCH] drivers/hv: remove deposited pages from direct map

[Magnus Kulke]

(RFC b/c feedback from arm64 maintainers to access the impact is
pending)

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 executed
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 the _noflush()
variant of set_direct_map_valid() 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>
---
 drivers/hv/hv_proc.c           | 53 +++++++++++++++++++++++++++++++++-
 drivers/hv/mshv_root_hv_call.c |  2 ++
 include/asm-generic/mshyperv.h |  4 +++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
index 57b2c64197cb..3ab02adf7efb 100644
--- a/drivers/hv/hv_proc.c
+++ b/drivers/hv/hv_proc.c
@@ -7,7 +7,9 @@
 #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>
 
 /*
  * See struct hv_deposit_memory. The first u64 is partition ID, the rest
@@ -15,6 +17,35 @@
  */
 #define HV_DEPOSIT_MAX (HV_HYP_PAGE_SIZE / sizeof(u64) - 1)
 
+/*
+ * Add or remove a set of physically contiguous page runs from the kernel
+ * direct map. Once a page has been deposited the hypervisor owns it and
+ * revokes root partition access to it.
+ */
+static int hv_deposit_update_direct_map(struct page **pages, int *counts,
+					int num_allocations, bool valid)
+{
+	int i, err, ret = 0;
+
+	for (i = 0; i < num_allocations; ++i) {
+		err = set_direct_map_valid_noflush(pages[i], counts[i], valid);
+		if (err && !ret)
+			ret = err;
+	}
+
+	if (valid)
+		return ret;
+
+	for (i = 0; i < num_allocations; ++i) {
+		unsigned long addr = (unsigned long)page_address(pages[i]);
+		unsigned long size = (unsigned long)counts[i] << PAGE_SHIFT;
+
+		flush_tlb_kernel_range(addr, addr + size);
+	}
+
+	return ret;
+}
+
 /* Deposits exact number of pages. Must be called with interrupts enabled.  */
 int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
 {
@@ -72,6 +103,10 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
 	}
 	num_allocations = i;
 
+	ret = hv_deposit_update_direct_map(pages, counts, num_allocations, false);
+	if (ret)
+		goto err_restore_direct_map;
+
 	local_irq_save(flags);
 
 	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
@@ -90,12 +125,15 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
 	if (!hv_result_success(status)) {
 		hv_status_err(status, "\n");
 		ret = hv_result_to_errno(status);
-		goto err_free_allocations;
+		goto err_restore_direct_map;
 	}
 
 	ret = 0;
 	goto free_buf;
 
+err_restore_direct_map:
+	hv_deposit_update_direct_map(pages, counts, num_allocations, true);
+
 err_free_allocations:
 	for (i = 0; i < num_allocations; ++i) {
 		base_pfn = page_to_pfn(pages[i]);
@@ -110,6 +148,19 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
 }
 EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
 
+/*
+ * Put withdrawn pages back in the direct map. Counterpart to the direct map
+ * removal done by hv_call_deposit_pages().
+ */
+void hv_restore_withdrawn_pages(const u64 *pfns, int count)
+{
+	int i;
+
+	for (i = 0; i < count; ++i)
+		set_direct_map_valid_noflush(pfn_to_page(pfns[i]), 1, true);
+}
+EXPORT_SYMBOL_GPL(hv_restore_withdrawn_pages);
+
 int hv_deposit_memory_node(int node, u64 partition_id,
 			   u64 hv_status)
 {
diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
index cb55d4d4be2e..8c21ad4ba3b0 100644
--- a/drivers/hv/mshv_root_hv_call.c
+++ b/drivers/hv/mshv_root_hv_call.c
@@ -69,6 +69,8 @@ int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
 
 		completed = hv_repcomp(status);
 
+		hv_restore_withdrawn_pages(output_page->gpa_page_list, completed);
+
 		for (i = 0; i < completed; i++)
 			__free_page(pfn_to_page(output_page->gpa_page_list[i]));
 
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index bf601d67cecb..397c8ec0ce9a 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -346,6 +346,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);
 int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages);
+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);
@@ -364,6 +365,9 @@ static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_page
 {
 	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: [RFC PATCH] drivers/hv: remove deposited pages from direct map

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Failing to restore direct map entries leads to unconditionally freeing unmapped pages into the buddy allocator, causing kernel crashes upon reallocation.
- [Low] The patch calls a non-existent kernel function `set_direct_map_valid_noflush`, which will cause a build failure.

Pre-existing issues:
- [High] Partial success of HVCALL_DEPOSIT_MEMORY causes deposited pages to be erroneously freed back to the buddy allocator.
--

commit 73ea756bd40436738445dd3d28edbd2b8f61584f
Author: Magnus Kulke <magnuskulke@linux.microsoft.com>

drivers/hv: remove deposited pages from direct map

This patch removes deposited pages from the kernel direct map before passing
them to the hypervisor, and restores them when the hypervisor returns them.
This aims to prevent kernel panics if code like load_unaligned_zeropad()
accidentally accesses these deposited pages.

> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 57b2c64197cb9..3ab02adf7efb4 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
[ ... ]
> +static int hv_deposit_update_direct_map(struct page **pages, int *counts,
> +					int num_allocations, bool valid)
> +{
> +	int i, err, ret = 0;
> +
> +	for (i = 0; i < num_allocations; ++i) {
> +		err = set_direct_map_valid_noflush(pages[i], counts[i], valid);

[Severity: Low]
Does this code call a function that doesn't exist? The kernel API does not
appear to have a set_direct_map_valid_noflush() function that takes three
arguments, which will cause a build failure.

> +		if (err && !ret)
> +			ret = err;
> +	}
> +
> +	if (valid)
> +		return ret;
[ ... ]
> @@ -90,12 +125,15 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  	if (!hv_result_success(status)) {
>  		hv_status_err(status, "\n");
>  		ret = hv_result_to_errno(status);
> -		goto err_free_allocations;
> +		goto err_restore_direct_map;
>  	}
>  
>  	ret = 0;
>  	goto free_buf;
>  
> +err_restore_direct_map:
> +	hv_deposit_update_direct_map(pages, counts, num_allocations, true);
> +
>  err_free_allocations:
>  	for (i = 0; i < num_allocations; ++i) {
>  		base_pfn = page_to_pfn(pages[i]);

[Severity: High]
This isn't a bug introduced by this patch, but does this error path properly
handle partially deposited pages?

If the HVCALL_DEPOSIT_MEMORY hypercall is interrupted or partially fails,
the hypervisor may have already taken ownership of some pages. The code does
not appear to check hv_repcomp(status) for partially deposited pages before
freeing them all to the buddy allocator. If the kernel dispenses these pages
again, any access could trigger a #GP fault from the hypervisor.

[Severity: High]
If restoring the direct map fails (for instance, due to an ENOMEM when trying
to split page tables), the error from hv_deposit_update_direct_map() is
ignored and execution proceeds to err_free_allocations. Could this free
unmapped pages to the buddy allocator, causing a crash when the kernel
later allocates and accesses them?

>  		for (j = 0; j < counts[i]; ++j)
>  			__free_page(pfn_to_page(base_pfn + j));
>  	}
[ ... ]
> @@ -110,6 +148,19 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  }
>  EXPORT_SYMBOL_GPL(hv_call_deposit_pages);
>  
> +/*
> + * Put withdrawn pages back in the direct map. Counterpart to the direct map
> + * removal done by hv_call_deposit_pages().
> + */
> +void hv_restore_withdrawn_pages(const u64 *pfns, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; ++i)
> +		set_direct_map_valid_noflush(pfn_to_page(pfns[i]), 1, true);

[Severity: Low]
Does this code call a non-existent function? Similar to the other location
in hv_deposit_update_direct_map(), this appears to use a non-existent
set_direct_map_valid_noflush() API with 3 arguments, which will break the
build.

[Severity: High]
Does this ignore the return value of the mapping restoration?

If the direct map restoration fails, the pages would remain unmapped in the
kernel's direct map. Ignoring this failure can cause the pages to be freed
unmapped into the buddy allocator by the caller.

> +}
> +EXPORT_SYMBOL_GPL(hv_restore_withdrawn_pages);
> +
>  int hv_deposit_memory_node(int node, u64 partition_id,
>  			   u64 hv_status)
>  {
> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> index cb55d4d4be2e5..8c21ad4ba3b07 100644
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
> @@ -69,6 +69,8 @@ int hv_call_withdraw_memory(u64 count, int node, u64 partition_id)
>  
>  		completed = hv_repcomp(status);
>  
> +		hv_restore_withdrawn_pages(output_page->gpa_page_list, completed);
> +
>  		for (i = 0; i < completed; i++)
>  			__free_page(pfn_to_page(output_page->gpa_page_list[i]));
>  

[Severity: High]
If hv_restore_withdrawn_pages() encounters an error, this loop will return
unmapped pages to the buddy allocator. Could this lead to a #PF and a kernel
panic when the buddy allocator dispenses these unmapped pages to another
subsystem?

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