Re: [PATCH V3 13/23] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Dec 2020, Oleksandr wrote:
> > On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > 
> > > The cmpxchg() in ioreq_send_buffered() operates on memory shared
> > > with the emulator domain (and the target domain if the legacy
> > > interface is used).
> > > 
> > > In order to be on the safe side we need to switch
> > > to guest_cmpxchg64() to prevent a domain to DoS Xen on Arm.
> > > 
> > > As there is no plan to support the legacy interface on Arm,
> > > we will have a page to be mapped in a single domain at the time,
> > > so we can use s->emulator in guest_cmpxchg64() safely.
> > > 
> > > Thankfully the only user of the legacy interface is x86 so far
> > > and there is not concern regarding the atomics operations.
> > > 
> > > Please note, that the legacy interface *must* not be used on Arm
> > > without revisiting the code.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > > Please note, this is a split/cleanup/hardening of Julien's PoC:
> > > "Add support for Guest IO forwarding to a device emulator"
> > > 
> > > Changes RFC -> V1:
> > >     - new patch
> > > 
> > > Changes V1 -> V2:
> > >     - move earlier to avoid breaking arm32 compilation
> > >     - add an explanation to commit description and hvm_allow_set_param()
> > >     - pass s->emulator
> > > 
> > > Changes V2 -> V3:
> > >     - update patch description
> > > ---
> > > ---
> > >   xen/arch/arm/hvm.c | 4 ++++
> > >   xen/common/ioreq.c | 3 ++-
> > >   2 files changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> > > index 8951b34..9694e5a 100644
> > > --- a/xen/arch/arm/hvm.c
> > > +++ b/xen/arch/arm/hvm.c
> > > @@ -31,6 +31,10 @@
> > >     #include <asm/hypercall.h>
> > >   +/*
> > > + * The legacy interface (which involves magic IOREQ pages) *must* not be
> > > used
> > > + * without revisiting the code.
> > > + */
> > This is a NIT, but I'd prefer if you moved the comment a few lines
> > below, maybe just before the existing comment starting with "The
> > following parameters".
> > 
> > The reason is that as it is now it is not clear which set_params
> > interfaces should not be used without revisiting the code.
> OK, but maybe this comment wants dropping at all? It was actual when the
> legacy interface was the part of the common code (V2). Now the legacy
> interface is
> x86 specific so I am not sure this comment should be here.

Yeah, fine by me.

 
> > 
> > With that:
> > 
> > Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Thank you