Message ID | 1606732298-22107-14-git-send-email-olekstysh@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [V3,01/23] x86/ioreq: Prepare IOREQ feature for making it common | expand |
On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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@epam.com> > CC: Julien Grall <julien.grall@arm.com> > > --- > 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@kernel.org> > 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 >
On 09.12.20 23:32, Stefano Stabellini wrote: Hi Stefano > On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> 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@epam.com> >> CC: Julien Grall <julien.grall@arm.com> >> >> --- >> 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. > > With that: > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> Thank you
On Thu, 10 Dec 2020, Oleksandr wrote: > > On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > > > > > 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@epam.com> > > > CC: Julien Grall <julien.grall@arm.com> > > > > > > --- > > > 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@kernel.org> > > Thank you
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. + */ 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);