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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2025 22:00, Stefano Stabellini wrote:
> 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."

It's the problem mentioned at the bottom: MFNs (in principle) are entirely
meaningless to HVM and PVH domains. With, as you point out there, an
apparently important exception.

>>> --- 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?

Once "shared" with the PSP, how would Xen know that this sharing has stopped?
Without knowing, how could it safely give the same page to a DomU later on?
("Safely" in both directions: Without compromising privacy of the DomU and
without compromising host safety / security.)

>> 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?

Yes, that's what immediately caught my eye. I didn't look carefully whether
there might be more.

>>> --- 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.

I see. That's pretty odd, though. I'm then further concerned of the order of
the chunks. At present we're rather lax, in permitting PVH and PV Dom0 the
same upper bound. With both CPU and I/O side translation there is, in
principle, no reason to permit any kind of contiguity. Of course there's a
performance aspect, but that hardly matters in the specific case here. Yet at
the same time, once we expose MFNs, contiguity will start mattering as soon
as any piece of memory needs to be larger than PAGE_SIZE. Which means it will
make tightening of the presently lax handling prone to regressions in this
new behavior you're introducing. What chunk size does the PSP driver require?

One further thought here: Is it really the hardware domain which is most
logical to drive the PSP? Interaction is (just guessing) perhaps needed
primarily when creating / managing guests? Then having the control domain
talk to a driver in the hardware domain would make the overall picture more
complicated.

Jan