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

[PATCH v2] 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 read/write RAM or logdirty.  That prevents the operation from
inadvertently changing the type as part of the move.

Fixes: 3e50af3d8776 ('New XENMAPSPACE_gmfn parameter for XENMEM_add_to_physmap.')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
 - Also handle logdirty types.
 - Return -ENOMEM on failure to unshare.
---
 xen/arch/x86/mm/p2m.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 2433230ac71c..759f3273d3d8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2015,11 +2015,17 @@ 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 read/write or logdirty RAM types
+         * to avoid unexpected p2m type changes as a result of the operation.
+         * Note that for logdirty source type we rely on p2m_add_page() marking
+         * the destination gfn as dirty.
+         */
+        if ( p2mt != p2m_ram_rw && p2mt != p2m_ram_logdirty )
         {
             put_gfn(d, gmfn);
-            return -ENOMEM;
+            return p2m_is_shared(p2mt) ? -ENOMEM : -EACCES;
         }
         page = get_page_from_mfn(mfn, d);
         if ( unlikely(!page) )
-- 
2.51.0

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

[Jan Beulich]

On 07.01.2026 21:32, 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 read/write RAM or logdirty.  That prevents the operation from
> inadvertently changing the type as part of the move.
> 
> Fixes: 3e50af3d8776 ('New XENMAPSPACE_gmfn parameter for XENMEM_add_to_physmap.')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since v1:
>  - Also handle logdirty types.
>  - Return -ENOMEM on failure to unshare.
> ---
>  xen/arch/x86/mm/p2m.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Since this is now ready to go in, the question of backporting arises. You
explicitly wanted the change here to only go in on top of what is now
98fccdf0ac7c ("x86/mm: update log-dirty bitmap when manipulating P2M"). I
wouldn't have considered that for backporting, but I guess for these two
it can only be both, neither, or said earlier commit shrunk to the minimum
required for the change here. Thoughts on which route to take?

Jan

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

[Roger Pau Monné]

On Thu, Jan 08, 2026 at 10:26:47AM +0100, Jan Beulich wrote:
> On 07.01.2026 21:32, 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 read/write RAM or logdirty.  That prevents the operation from
> > inadvertently changing the type as part of the move.
> > 
> > Fixes: 3e50af3d8776 ('New XENMAPSPACE_gmfn parameter for XENMEM_add_to_physmap.')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > Changes since v1:
> >  - Also handle logdirty types.
> >  - Return -ENOMEM on failure to unshare.
> > ---
> >  xen/arch/x86/mm/p2m.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Since this is now ready to go in, the question of backporting arises. You
> explicitly wanted the change here to only go in on top of what is now
> 98fccdf0ac7c ("x86/mm: update log-dirty bitmap when manipulating P2M"). I
> wouldn't have considered that for backporting, but I guess for these two
> it can only be both, neither, or said earlier commit shrunk to the minimum
> required for the change here. Thoughts on which route to take?

Maybe shrink 98fccdf0ac7c so it only contains the added
paging_mark_pfn_dirty() calls (not the clean ones)?  That would be
enough for the dependency this patch has on 98fccdf0ac7c.  I think
this would be my preference.

Otherwise we could backport just this one, and loose the correct
handling of dirty GFN, which TBH is already the case on all releases
(that don't contain 98fccdf0ac7c).  We won't be making the code
strictly worse, but we won't also be fully fixing it.

Thanks, Roger.