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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

With that:

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>  static int hvm_allow_set_param(const struct domain *d, unsigned int param)
>  {
>      switch ( param )
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index 3ca5b96..4855dd8 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -29,6 +29,7 @@
>  #include <xen/trace.h>
>  #include <xen/vpci.h>
>  
> +#include <asm/guest_atomics.h>
>  #include <asm/hvm/ioreq.h>
>  
>  #include <public/hvm/ioreq.h>
> @@ -1182,7 +1183,7 @@ static int ioreq_send_buffered(struct ioreq_server *s, 
> ioreq_t *p)
>  
>          new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>          new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> +        guest_cmpxchg64(s->emulator, &pg->ptrs.full, old.full, new.full);
>      }
>  
>      notify_via_xen_event_channel(d, s->bufioreq_evtchn);
> -- 
> 2.7.4
>