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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 24/11/2022 9:29 pm, Julien Grall wrote:
> Hi Jan,
>
> I am still digesting this series and replying with some initial comments.
>
> 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.

~Andrew