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

[dmkhn@xxxxxxxxx]

On Sat, Jul 26, 2025 at 01:26:07PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ----
> 
> 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
> 
>