[PATCH] P2M: Don't try to free the existing PTE if we can't allocate a new table

[PATCH] P2M: Don't try to free the existing PTE if we can't allocate a new table

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

When we can't split a superpage (on Arm p2m_split_superpage() returns false,
on x86 ept_split_superpage() returns 0), the caller is expected to clean
any PTE that may have been allocated. However, when we can't allocate
the page-tables 'entry' (arm) / 'ept_entry' still points to a live PTE.
So we will end up to free a PTE that is still used.

In practice for:
  * x86: We don't do any refcounting for 2MB/1GB mappings. So this is
    harmless
  * arm: We do refcounting for 2MB mapping (not for 1GB ones). This is
    only used for static memory.

So there is a security issue on Arm but this doesn't meet the criteria
for an XSA (static memory is not security supported).

Solve the issue by clearing the PTE if we can't allocate the table.

Reported-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

----

I decided to not split the patch in two as the issue for x86 and
arm is the same. But I am happy to split if this is preferred.
---
 xen/arch/arm/mmu/p2m.c    | 8 ++++++++
 xen/arch/x86/mm/p2m-ept.c | 9 +++++++++
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index 51abf3504fcf..9a1fb44a0204 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -888,7 +888,15 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
 
     page = p2m_alloc_page(p2m->domain);
     if ( !page )
+    {
+        /*
+         * The caller is in charge to free the sub-tree. So tell the
+         * As we didn't manage to allocate anything, just tell the
+         * caller there is nothing to free by invalidating the PTE.
+         */
+        memset(entry, 0, sizeof(*entry));
         return false;
+    }
 
     page_list_add(page, &p2m->pages);
     table = __map_domain_page(page);
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 62fc8e50689f..1efac27835d2 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -261,7 +261,16 @@ static bool ept_split_super_page(
 
     table = ept_set_middle_entry(p2m, &new_ept);
     if ( !table )
+    {
+        /*
+         * The caller is in charge to free the sub-tree. So tell the
+         * As we didn't manage to allocate anything, just tell the
+         * caller there is nothing to free by invalidating the PTE.
+         */
+        memset(ept_entry, 0, sizeof(*ept_entry));
+
         return 0;
+    }
 
     trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
 
-- 
2.47.3

Re: [PATCH] P2M: Don't try to free the existing PTE if we can't allocate a new table

[dmkhn]

On Sat, Jul 26, 2025 at 01:26:07PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> When we can't split a superpage (on Arm p2m_split_superpage() returns false,
> on x86 ept_split_superpage() returns 0), the caller is expected to clean
> any PTE that may have been allocated. However, when we can't allocate
> the page-tables 'entry' (arm) / 'ept_entry' still points to a live PTE.
> So we will end up to free a PTE that is still used.
> 
> In practice for:
>   * x86: We don't do any refcounting for 2MB/1GB mappings. So this is
>     harmless
>   * arm: We do refcounting for 2MB mapping (not for 1GB ones). This is
>     only used for static memory.
> 
> So there is a security issue on Arm but this doesn't meet the criteria
> for an XSA (static memory is not security supported).
> 
> Solve the issue by clearing the PTE if we can't allocate the table.
> 
> Reported-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> I decided to not split the patch in two as the issue for x86 and
> arm is the same. But I am happy to split if this is preferred.
> ---
>  xen/arch/arm/mmu/p2m.c    | 8 ++++++++
>  xen/arch/x86/mm/p2m-ept.c | 9 +++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 51abf3504fcf..9a1fb44a0204 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -888,7 +888,15 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
> 
>      page = p2m_alloc_page(p2m->domain);
>      if ( !page )
> +    {
> +        /*
> +         * The caller is in charge to free the sub-tree. So tell the

                                                            ^^^
Looks like the "So tell the" can be dropped from the commentary.
Same comment for the p2m-ept.c below.

> +         * As we didn't manage to allocate anything, just tell the
> +         * caller there is nothing to free by invalidating the PTE.
> +         */
> +        memset(entry, 0, sizeof(*entry));
>          return false;
> +    }
> 
>      page_list_add(page, &p2m->pages);
>      table = __map_domain_page(page);
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 62fc8e50689f..1efac27835d2 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -261,7 +261,16 @@ static bool ept_split_super_page(
> 
>      table = ept_set_middle_entry(p2m, &new_ept);
>      if ( !table )
> +    {
> +        /*
> +         * The caller is in charge to free the sub-tree. So tell the
> +         * As we didn't manage to allocate anything, just tell the
> +         * caller there is nothing to free by invalidating the PTE.
> +         */
> +        memset(ept_entry, 0, sizeof(*ept_entry));
> +
>          return 0;
> +    }
> 
>      trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
> 
> --
> 2.47.3
> 
>