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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 12/01/2021 21:52, 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.
I think you want to explain why you are using the 64-bit version of helper.

> 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>
> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> [On Arm only]
> Tested-by: Wei Chen <Wei.Chen@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
>
> Changes V3 -> V4:
>     - add Stefano's A-b
>     - drop comment from arm/hvm.c
> ---
>   xen/common/ioreq.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index d233a49..d5f4dd3 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>
> @@ -1185,7 +1186,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);
Cheers,

--
Julien Grall