Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2023 23:20, Andrew Cooper wrote:
> On 24/11/2022 9:29 pm, Julien Grall wrote:
>> On 19/10/2022 09:43, Jan Beulich wrote:
>>> The registration by virtual/linear address has downsides: At least on
>>> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
>>> PV domains the areas are inaccessible (and hence cannot be updated by
>>> Xen) when in guest-user mode.
>>>
>>> In preparation of the introduction of new vCPU operations allowing to
>>> register the respective areas (one of the two is x86-specific) by
>>> guest-physical address, flesh out the map/unmap functions.
>>>
>>> Noteworthy differences from map_vcpu_info():
>>> - areas can be registered more than once (and de-registered),
>>> - remote vCPU-s are paused rather than checked for being down (which in
>>>    principle can change right after the check),
>>> - the domain lock is taken for a much smaller region.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> RFC: By using global domain page mappings the demand on the underlying
>>>       VA range may increase significantly. I did consider to use per-
>>>       domain mappings instead, but they exist for x86 only. Of course we
>>>       could have arch_{,un}map_guest_area() aliasing global domain page
>>>       mapping functions on Arm and using per-domain mappings on x86. Yet
>>>       then again map_vcpu_info() doesn't do so either (albeit that's
>>>       likely to be converted subsequently to use map_vcpu_area()
>>> anyway).
>>>
>>> RFC: In map_guest_area() I'm not checking the P2M type, instead - just
>>>       like map_vcpu_info() - solely relying on the type ref acquisition.
>>>       Checking for p2m_ram_rw alone would be wrong, as at least
>>>       p2m_ram_logdirty ought to also be okay to use here (and in similar
>>>       cases, e.g. in Argo's find_ring_mfn()). p2m_is_pageable() could be
>>>       used here (like altp2m_vcpu_enable_ve() does) as well as in
>>>       map_vcpu_info(), yet then again the P2M type is stale by the time
>>>       it is being looked at anyway without the P2M lock held.
>>>
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr
>>>                      struct guest_area *area,
>>>                      void (*populate)(void *dst, struct vcpu *v))
>>>   {
>>> -    return -EOPNOTSUPP;
>>> +    struct domain *currd = v->domain;
>>> +    void *map = NULL;
>>> +    struct page_info *pg = NULL;
>>> +    int rc = 0;
>>> +
>>> +    if ( gaddr )
>>
>> 0 is technically a valid (guest) physical address on Arm.
> 
> It is on x86 too, but that's not why 0 is generally considered an
> invalid address.
> 
> See the multitude of XSAs, and near-XSAs which have been caused by bad
> logic in Xen caused by trying to make a variable held in struct
> vcpu/domain have a default value other than 0.
> 
> It's not impossible to write such code safely, and in this case I expect
> it can be done by the NULLness (or not) of the mapping pointer, rather
> than by stashing the gaddr, but history has proved repeatedly that this
> is a very fertile source of security bugs.

I'm checking a value passed in from the guest here. No checking of internal
state can replace that. The checks on internal state leverage zero-init:

 unmap:
    if ( pg )
    {
        unmap_domain_page_global(map);
        put_page_and_type(pg);
    }

It's also not clear to me whether, like Julien looks to have read it, you
mean to ask that I revert back to using 0 as the "invalid" (i.e. request
for unmap) indicator.

Jan