Re: [PATCH v5 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Julien Grall <julien@xxxxxxx>]

On 07/07/2021 15:06, Jan Beulich wrote:
> On 07.07.2021 15:39, Julien Grall wrote:
> > On 05/07/2021 09:41, Jan Beulich wrote:
> > > On 03.07.2021 19:11, Julien Grall wrote:
> > > > Changes in v5:
> > > >       - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
> > > >       - Used paging_mode_translate() rather than is_pv_domain()
> > > Is there a particular reason you use this in favor of steal_page()'s
> > > paging_mode_external()?
> > This is what you suggested in v4 [1]. I can switch to
> > paging_mode_external() if this is what you now prefer.
> Well, I did say this would be better than is_pv_*(). I probably didn't
> pay enough attention to you already pointing out paging_mode_external()
> in the description; I'm sorry. On x86 both are in sync anyway, and I
> have to admit I don't see clearly enough which of the two would be the
> right one to use here, partly because I'm afraid I also don't see why
> steal_page() has such a restriction in the first place (which you now
> build upon).
From a quick git blame, I have found this:

commit fae7d5be8bb8b7a5b7005c4f3b812a47661a721e
Author: Jan Beulich <jbeulich@xxxxxxxx>
Date:   Tue Jun 20 14:29:51 2017 +0200

    x86/mm: disallow page stealing from HVM domains

    The operation's success can't be controlled by the guest, as the device
    model may have an active mapping of the page. If we nevertheless
    permitted this operation, we'd have to add further TLB flushing to
    prevent scenarios like

    "Domains A (HVM), B (PV), C (PV); B->target==A
     Steps:
     1. B maps page X from A as writable
     2. B unmaps page X without a TLB flush
     3. A sends page X to C via GNTTABOP_transfer
     4. C maps page X as pagetable (potentially causing a TLB flush in C,
     but not in B)

     At this point, X would be mapped as a pagetable in C while being
     writable through a stale TLB entry in B."

    A similar scenario could be constructed for A using XENMEM_exchange and
    some arbitrary PV domain C then having this page allocated.

    This is XSA-217.

    Reported-by: Jann Horn <jannh@xxxxxxxxxx>
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
    Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

> > > > @@ -815,6 +812,9 @@ static long 
> > > > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> > > >        if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
> > > >            rc = -EFAULT;
> > > I'm afraid that for correctness of the interface you need to keep
> > > this part even in the !PV case.
> > Xen never initializes the field nr_exchanged. Instead, it expects the
> > guest to set to 0. So I am not quite to sure why we would need to keep
> > this line.
> Hmm, the public header is wrong then, as it documents the field as
> [OUT] only _despite_ the shouting warning in point 5 of the comment.
That's confusing... I will look to update the doc.

> I guess I never really understood why this sub-op differs from
> others in where the continuation indicator lives.
I am guessing the continuation was added after the fact without 
coordination?
> Never mind then, indeed no code adjustment needed:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks!

Cheers,

--
Julien Grall