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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 18/01/2023 9:59 am, Jan Beulich wrote:
> 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.

I'm merely asking you to be extra careful and not add bugs to the error
paths.  And it appears that you have done it based on the NULLness of
the mapping pointer, which is fine.

~Andrew