Re: [PATCH] xen/x86: allow Dom0 PVH to call XENMEM_exchange

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 28 Apr 2025, Jan Beulich wrote:
> On 25.04.2025 22:19, Stefano Stabellini wrote:
> > From: Xenia Ragiadakou <Xenia.Ragiadakou@xxxxxxx>
> > 
> > Dom0 PVH might need XENMEM_exchange when passing contiguous memory
> > addresses to firmware or co-processors not behind an IOMMU.
> 
> I definitely don't understand the firmware part: It's subject to the
> same transparent P2M translations as the rest of the VM; it's just
> another piece of software running there.
> 
> "Co-processors not behind an IOMMU" is also interesting; a more
> concrete scenario might be nice, yet I realize you may be limited in
> what you're allowed to say.

Sure. On AMD x86 platforms there is a co-processor called PSP running
TEE firmware. The PSP is not behind an IOMMU. Dom0 needs occasionally to
pass addresses to it.  See drivers/tee/amdtee/ and
include/linux/psp-tee.h in Linux.

This is not a new problem. On ARM we have been dealing with this kind of
issues for more than a decade and it is the reason why originally the
decision was to run Dom0 1:1 mapped on ARM. I am not suggesting to map
Dom0 PVH 1:1; I am only providing context and highlighting that we
have been lucky on x86 :-)


> > XENMEM_exchange was blocked for HVM/PVH DomUs, and accidentally it
> > impacted Dom0 PVH as well.
> 
> This wasn't accidental at all, I don't think.

You as the original author of the patch (fae7d5be8bb) in question would
surely know better. But the way the commit message was written was
pointing toward a Dom0/DeviceModel problem:

"The operation's success can't be controlled by the guest, as the device
model may have an active mapping of the page."


> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4401,7 +4401,7 @@ int steal_page(
> >      const struct domain *owner;
> >      int rc;
> >  
> > -    if ( paging_mode_external(d) )
> > +    if ( paging_mode_external(d) && !is_hardware_domain(d) )
> >          return -EOPNOTSUPP;
> >  
> >      /* Grab a reference to make sure the page doesn't change under our 
> > feet */
> 
> Is this (in particular the code following below here) a safe thing to do
> when we don't properly refcount page references from the P2M, yet? It's
> Dom0, yes, but even there I might see potential security implications (as
> top violating privacy of a guest).

I don't think I am following, could you please elaborate more? The
change I am proposing is to allow Dom0 to share its own pages to the
co-processor. DomUs are not in the picture. I would be happy to add
further restriction to that effect. Is there something else you have in
mind?


> Furthermore cleanup_page_mappings() (called later in the function) has a
> PV-only aspect which would apparently need widening to PVH Dom0 then,
> too.

You are referring to:

        if ( d && unlikely(need_iommu_pt_sync(d)) && is_pv_domain(d) )
            rc = iommu_legacy_unmap(d, _dfn(mfn), 1u << PAGE_ORDER_4K);

is that correct?


> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -794,7 +794,7 @@ static long 
> > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >              rc = guest_physmap_add_page(d, _gfn(gpfn), mfn,
> >                                          exch.out.extent_order) ?: rc;
> >  
> > -            if ( !paging_mode_translate(d) &&
> > +            if ( (!paging_mode_translate(d) || is_hardware_domain(d)) &&
> >                   __copy_mfn_to_guest_offset(exch.out.extent_start,
> >                                              (i << out_chunk_order) + j,
> >                                              mfn) )
> 
> Wait, no: A PVH domain (Dom0 or not) can't very well make use of MFNs, can
> it?

One way or another Dom0 PVH needs to know the MFN to pass it to the
co-processor.