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 <jbeul...@suse.com>
>>> ---
>>> 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
