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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 17/01/2023 22:20, Andrew Cooper wrote:
> 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.
I understand the security concern. However... you are now imposing some 
constraint in the guest OS which may be more difficult to address.
Furthermore, we are trying to make a sane ABI. It doesn't look sane to 
me to expose our currently shortcomings to the guest OS. The more that 
if we decide it to relax in the future, then it would not help an OS 
because they will need to support older Xen...
Cheers,

--
Julien Grall