[PATCH] x86/hvm: be more strict with XENMAPSPACE_gmfn source types

[PATCH] x86/hvm: be more strict with XENMAPSPACE_gmfn source types

[Roger Pau Monne]

XENMAPSPACE_gmfn{_range} allows moving gfn around the guest p2m: the mfn
behind the source gfn is zapped from the origin and mapped at the
requested destination gfn.  The destination p2m entries are always created
with type p2m_ram_rw.

With the current checking done in xenmem_add_to_physmap_one() it's possible
to use XENMAPSPACE_gmfn{_range} to change the type of a p2m entry.  The
source gfn is only checked to be not shared, and that the underlying page
is owned by the domain.

Make the source checks more strict, by checking that the source gfn is of
type p2m_ram_rw.  That prevents the operation from inadvertently changing
the type as part of the move.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The change was discussed internally by the security team and deemed not a
security issue.
---
 xen/arch/x86/mm/p2m.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e2a00a0efd0c..452b2f8f0f10 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2007,11 +2007,15 @@ int xenmem_add_to_physmap_one(
     {
         gmfn = idx;
         mfn = get_gfn_unshare(d, gmfn, &p2mt);
-        /* If the page is still shared, exit early */
-        if ( p2m_is_shared(p2mt) )
+        /*
+         * The entry at the destination gfn will be created as type p2m_ram_rw.
+         * Only allow moving source gfns with p2m_ram_rw type to avoid
+         * unexpected p2m type changes as a result of the operation.
+         */
+        if ( p2mt != p2m_ram_rw )
         {
             put_gfn(d, gmfn);
-            return -ENOMEM;
+            return -EACCES;
         }
         page = get_page_from_mfn(mfn, d);
         if ( unlikely(!page) )
-- 
2.51.0

Re: [PATCH] x86/hvm: be more strict with XENMAPSPACE_gmfn source types

[Jan Beulich]

On 05.12.2025 10:31, Roger Pau Monne wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2007,11 +2007,15 @@ int xenmem_add_to_physmap_one(
>      {
>          gmfn = idx;
>          mfn = get_gfn_unshare(d, gmfn, &p2mt);
> -        /* If the page is still shared, exit early */
> -        if ( p2m_is_shared(p2mt) )
> +        /*
> +         * The entry at the destination gfn will be created as type p2m_ram_rw.
> +         * Only allow moving source gfns with p2m_ram_rw type to avoid
> +         * unexpected p2m type changes as a result of the operation.
> +         */
> +        if ( p2mt != p2m_ram_rw )

As asked before - what about p2m_log_dirty? Imo that needs permitting here
as well. Making it become p2m_ram_rw is "natural", as long as the (new) GFN
is suitably marked dirty (which p2m_add_page() looks to be doing).

>          {
>              put_gfn(d, gmfn);
> -            return -ENOMEM;
> +            return -EACCES;

Since we tried to unshare, imo ENOMEM should continue to be returned for
p2m_is_shared().

With both changes:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/hvm: be more strict with XENMAPSPACE_gmfn source types

[Roger Pau Monné]

On Fri, Dec 05, 2025 at 10:31:51AM +0100, Roger Pau Monne wrote:
> XENMAPSPACE_gmfn{_range} allows moving gfn around the guest p2m: the mfn
> behind the source gfn is zapped from the origin and mapped at the
> requested destination gfn.  The destination p2m entries are always created
> with type p2m_ram_rw.
> 
> With the current checking done in xenmem_add_to_physmap_one() it's possible
> to use XENMAPSPACE_gmfn{_range} to change the type of a p2m entry.  The
> source gfn is only checked to be not shared, and that the underlying page
> is owned by the domain.
> 
> Make the source checks more strict, by checking that the source gfn is of
> type p2m_ram_rw.  That prevents the operation from inadvertently changing
> the type as part of the move.

This is missing:

Fixes: 3e50af3d8776 ('New XENMAPSPACE_gmfn parameter for XENMEM_add_to_physmap.')

The hypercall was missing any p2m type checks since introduction.
It's possible the get_page() seemed enough, but it was dangerous to
not account for new incompatible p2m types being added down the road.

Thanks, Roger.