* [PATCH] Fix ioreq-server event channel vulnerability
@ 2014-07-14 10:21 Paul Durrant
2014-07-14 13:53 ` Andrew Cooper
0 siblings, 1 reply; 3+ messages in thread
From: Paul Durrant @ 2014-07-14 10:21 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Keir Fraser, Jan Beulich
The code in hvm_send_assist_req_to_ioreq_server() and hvm_do_resume() uses
an event channel port number taken from the page of memory shared with the
emulator. This allows an emulator to corrupt values that are then blindly
used by Xen, leading to assertion failures in some cases. Moreover, in the
case of the default ioreq server the page remains in the guest p2m so a
malicious guest could similarly corrupt those values.
This patch changes the afforementioned functions to get the event channel
port number from an internal structure and also adds an extra check to
hvm_send_assist_req_to_ioreq_server() which will crash the domain should the
guest or an emulator corrupt the port number in the shared page.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reported-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/hvm.c | 112 ++++++++++++++++++++++++++++++++----------------
1 file changed, 75 insertions(+), 37 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ef2411c..9a09ee1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -392,6 +392,31 @@ bool_t hvm_io_pending(struct vcpu *v)
return 0;
}
+static void hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
+{
+ /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
+ while ( p->state != STATE_IOREQ_NONE )
+ {
+ switch ( p->state )
+ {
+ case STATE_IORESP_READY: /* IORESP_READY -> NONE */
+ rmb(); /* see IORESP_READY /then/ read contents of ioreq */
+ hvm_io_assist(p);
+ break;
+ case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY */
+ case STATE_IOREQ_INPROCESS:
+ wait_on_xen_event_channel(sv->ioreq_evtchn,
+ (p->state != STATE_IOREQ_READY) &&
+ (p->state != STATE_IOREQ_INPROCESS));
+ break;
+ default:
+ gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p->state);
+ domain_crash(sv->vcpu->domain);
+ return; /* bail */
+ }
+ }
+}
+
void hvm_do_resume(struct vcpu *v)
{
struct domain *d = v->domain;
@@ -406,27 +431,17 @@ void hvm_do_resume(struct vcpu *v)
&d->arch.hvm_domain.ioreq_server.list,
list_entry )
{
- ioreq_t *p = get_ioreq(s, v);
+ struct hvm_ioreq_vcpu *sv;
- /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
- while ( p->state != STATE_IOREQ_NONE )
+ list_for_each_entry ( sv,
+ &s->ioreq_vcpu_list,
+ list_entry )
{
- switch ( p->state )
+ if ( sv->vcpu == v )
{
- case STATE_IORESP_READY: /* IORESP_READY -> NONE */
- rmb(); /* see IORESP_READY /then/ read contents of ioreq */
- hvm_io_assist(p);
- break;
- case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY */
- case STATE_IOREQ_INPROCESS:
- wait_on_xen_event_channel(p->vp_eport,
- (p->state != STATE_IOREQ_READY) &&
- (p->state != STATE_IOREQ_INPROCESS));
- break;
- default:
- gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p->state);
- domain_crash(d);
- return; /* bail */
+ ioreq_t *p = get_ioreq(s, v);
+
+ hvm_wait_for_io(sv, p);
}
}
}
@@ -2545,35 +2560,58 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
{
struct vcpu *curr = current;
struct domain *d = curr->domain;
- ioreq_t *p;
+ struct hvm_ioreq_vcpu *sv;
if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
return 0; /* implicitly bins the i/o operation */
- p = get_ioreq(s, curr);
-
- if ( unlikely(p->state != STATE_IOREQ_NONE) )
+ list_for_each_entry ( sv,
+ &s->ioreq_vcpu_list,
+ list_entry )
{
- /* This indicates a bug in the device model. Crash the domain. */
- gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n", p->state);
- domain_crash(d);
- return 0;
- }
+ if ( sv->vcpu == curr )
+ {
+ evtchn_port_t port = sv->ioreq_evtchn;
+ ioreq_t *p = get_ioreq(s, curr);
- proto_p->state = STATE_IOREQ_NONE;
- proto_p->vp_eport = p->vp_eport;
- *p = *proto_p;
+ if ( unlikely(p->state != STATE_IOREQ_NONE) )
+ {
+ gdprintk(XENLOG_ERR,
+ "Device model set bad IO state %d.\n",
+ p->state);
+ goto crash;
+ }
- prepare_wait_on_xen_event_channel(p->vp_eport);
+ if ( unlikely(p->vp_eport != port) )
+ {
+ gdprintk(XENLOG_ERR,
+ "Device model set bad event channel %d.\n",
+ p->vp_eport);
+ goto crash;
+ }
- /*
- * Following happens /after/ blocking and setting up ioreq contents.
- * prepare_wait_on_xen_event_channel() is an implicit barrier.
- */
- p->state = STATE_IOREQ_READY;
- notify_via_xen_event_channel(d, p->vp_eport);
+ proto_p->state = STATE_IOREQ_NONE;
+ proto_p->vp_eport = port;
+ *p = *proto_p;
+
+ prepare_wait_on_xen_event_channel(port);
+
+ /*
+ * Following happens /after/ blocking and setting up ioreq
+ * contents. prepare_wait_on_xen_event_channel() is an implicit
+ * barrier.
+ */
+ p->state = STATE_IOREQ_READY;
+ notify_via_xen_event_channel(d, port);
+ break;
+ }
+ }
return 1;
+
+ crash:
+ domain_crash(d);
+ return 0;
}
bool_t hvm_send_assist_req(ioreq_t *p)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] Fix ioreq-server event channel vulnerability
2014-07-14 10:21 [PATCH] Fix ioreq-server event channel vulnerability Paul Durrant
@ 2014-07-14 13:53 ` Andrew Cooper
2014-07-14 13:58 ` Paul Durrant
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2014-07-14 13:53 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Keir Fraser, Jan Beulich
On 14/07/14 11:21, Paul Durrant wrote:
> The code in hvm_send_assist_req_to_ioreq_server() and hvm_do_resume() uses
> an event channel port number taken from the page of memory shared with the
> emulator. This allows an emulator to corrupt values that are then blindly
> used by Xen, leading to assertion failures in some cases. Moreover, in the
> case of the default ioreq server the page remains in the guest p2m so a
> malicious guest could similarly corrupt those values.
>
> This patch changes the afforementioned functions to get the event channel
> port number from an internal structure and also adds an extra check to
> hvm_send_assist_req_to_ioreq_server() which will crash the domain should the
> guest or an emulator corrupt the port number in the shared page.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reported-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/arch/x86/hvm/hvm.c | 112 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 75 insertions(+), 37 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef2411c..9a09ee1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -392,6 +392,31 @@ bool_t hvm_io_pending(struct vcpu *v)
> return 0;
> }
>
> +static void hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
> +{
> + /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
> + while ( p->state != STATE_IOREQ_NONE )
> + {
> + switch ( p->state )
> + {
> + case STATE_IORESP_READY: /* IORESP_READY -> NONE */
> + rmb(); /* see IORESP_READY /then/ read contents of ioreq */
> + hvm_io_assist(p);
> + break;
> + case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY */
> + case STATE_IOREQ_INPROCESS:
> + wait_on_xen_event_channel(sv->ioreq_evtchn,
> + (p->state != STATE_IOREQ_READY) &&
> + (p->state != STATE_IOREQ_INPROCESS));
> + break;
> + default:
> + gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p->state);
> + domain_crash(sv->vcpu->domain);
> + return; /* bail */
> + }
> + }
> +}
> +
> void hvm_do_resume(struct vcpu *v)
> {
> struct domain *d = v->domain;
> @@ -406,27 +431,17 @@ void hvm_do_resume(struct vcpu *v)
> &d->arch.hvm_domain.ioreq_server.list,
> list_entry )
> {
> - ioreq_t *p = get_ioreq(s, v);
> + struct hvm_ioreq_vcpu *sv;
>
> - /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE). */
> - while ( p->state != STATE_IOREQ_NONE )
> + list_for_each_entry ( sv,
> + &s->ioreq_vcpu_list,
> + list_entry )
> {
> - switch ( p->state )
> + if ( sv->vcpu == v )
> {
> - case STATE_IORESP_READY: /* IORESP_READY -> NONE */
> - rmb(); /* see IORESP_READY /then/ read contents of ioreq */
> - hvm_io_assist(p);
> - break;
> - case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} -> IORESP_READY */
> - case STATE_IOREQ_INPROCESS:
> - wait_on_xen_event_channel(p->vp_eport,
> - (p->state != STATE_IOREQ_READY) &&
> - (p->state != STATE_IOREQ_INPROCESS));
> - break;
> - default:
> - gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p->state);
> - domain_crash(d);
> - return; /* bail */
By pulling this return into hvm_wait_for_io(), you now continue through
hvm_do_resume() even after deciding to crash the domain, which has
changed the error behaviour and doesn't look like it will go well with
hvm_inject_trap() out of context below. You probably want a boolean
return from hvm_wait_for_io().
> + ioreq_t *p = get_ioreq(s, v);
> +
> + hvm_wait_for_io(sv, p);
You presumably want to break at this point, or you will pointlessly
continue the list_for_each_entry() loop. You can also get away with
folding these 3 lines together and doing away with 'p'.
~Andrew
> }
> }
> }
> @@ -2545,35 +2560,58 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
> {
> struct vcpu *curr = current;
> struct domain *d = curr->domain;
> - ioreq_t *p;
> + struct hvm_ioreq_vcpu *sv;
>
> if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> return 0; /* implicitly bins the i/o operation */
>
> - p = get_ioreq(s, curr);
> -
> - if ( unlikely(p->state != STATE_IOREQ_NONE) )
> + list_for_each_entry ( sv,
> + &s->ioreq_vcpu_list,
> + list_entry )
> {
> - /* This indicates a bug in the device model. Crash the domain. */
> - gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n", p->state);
> - domain_crash(d);
> - return 0;
> - }
> + if ( sv->vcpu == curr )
> + {
> + evtchn_port_t port = sv->ioreq_evtchn;
> + ioreq_t *p = get_ioreq(s, curr);
>
> - proto_p->state = STATE_IOREQ_NONE;
> - proto_p->vp_eport = p->vp_eport;
> - *p = *proto_p;
> + if ( unlikely(p->state != STATE_IOREQ_NONE) )
> + {
> + gdprintk(XENLOG_ERR,
> + "Device model set bad IO state %d.\n",
> + p->state);
> + goto crash;
> + }
>
> - prepare_wait_on_xen_event_channel(p->vp_eport);
> + if ( unlikely(p->vp_eport != port) )
> + {
> + gdprintk(XENLOG_ERR,
> + "Device model set bad event channel %d.\n",
> + p->vp_eport);
> + goto crash;
> + }
>
> - /*
> - * Following happens /after/ blocking and setting up ioreq contents.
> - * prepare_wait_on_xen_event_channel() is an implicit barrier.
> - */
> - p->state = STATE_IOREQ_READY;
> - notify_via_xen_event_channel(d, p->vp_eport);
> + proto_p->state = STATE_IOREQ_NONE;
> + proto_p->vp_eport = port;
> + *p = *proto_p;
> +
> + prepare_wait_on_xen_event_channel(port);
> +
> + /*
> + * Following happens /after/ blocking and setting up ioreq
> + * contents. prepare_wait_on_xen_event_channel() is an implicit
> + * barrier.
> + */
> + p->state = STATE_IOREQ_READY;
> + notify_via_xen_event_channel(d, port);
> + break;
> + }
> + }
>
> return 1;
> +
> + crash:
> + domain_crash(d);
> + return 0;
> }
>
> bool_t hvm_send_assist_req(ioreq_t *p)
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] Fix ioreq-server event channel vulnerability
2014-07-14 13:53 ` Andrew Cooper
@ 2014-07-14 13:58 ` Paul Durrant
0 siblings, 0 replies; 3+ messages in thread
From: Paul Durrant @ 2014-07-14 13:58 UTC (permalink / raw)
To: Andrew Cooper, xen-devel@lists.xen.org; +Cc: Keir (Xen.org), Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 14 July 2014 14:53
> To: Paul Durrant; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH] Fix ioreq-server event channel vulnerability
>
> On 14/07/14 11:21, Paul Durrant wrote:
> > The code in hvm_send_assist_req_to_ioreq_server() and
> hvm_do_resume() uses
> > an event channel port number taken from the page of memory shared
> with the
> > emulator. This allows an emulator to corrupt values that are then blindly
> > used by Xen, leading to assertion failures in some cases. Moreover, in the
> > case of the default ioreq server the page remains in the guest p2m so a
> > malicious guest could similarly corrupt those values.
> >
> > This patch changes the afforementioned functions to get the event
> channel
> > port number from an internal structure and also adds an extra check to
> > hvm_send_assist_req_to_ioreq_server() which will crash the domain
> should the
> > guest or an emulator corrupt the port number in the shared page.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reported-by: Wen Congyang <wency@cn.fujitsu.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > xen/arch/x86/hvm/hvm.c | 112
> ++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 75 insertions(+), 37 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef2411c..9a09ee1 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -392,6 +392,31 @@ bool_t hvm_io_pending(struct vcpu *v)
> > return 0;
> > }
> >
> > +static void hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
> > +{
> > + /* NB. Optimised for common case (p->state == STATE_IOREQ_NONE).
> */
> > + while ( p->state != STATE_IOREQ_NONE )
> > + {
> > + switch ( p->state )
> > + {
> > + case STATE_IORESP_READY: /* IORESP_READY -> NONE */
> > + rmb(); /* see IORESP_READY /then/ read contents of ioreq */
> > + hvm_io_assist(p);
> > + break;
> > + case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} ->
> IORESP_READY */
> > + case STATE_IOREQ_INPROCESS:
> > + wait_on_xen_event_channel(sv->ioreq_evtchn,
> > + (p->state != STATE_IOREQ_READY) &&
> > + (p->state != STATE_IOREQ_INPROCESS));
> > + break;
> > + default:
> > + gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p-
> >state);
> > + domain_crash(sv->vcpu->domain);
> > + return; /* bail */
> > + }
> > + }
> > +}
> > +
> > void hvm_do_resume(struct vcpu *v)
> > {
> > struct domain *d = v->domain;
> > @@ -406,27 +431,17 @@ void hvm_do_resume(struct vcpu *v)
> > &d->arch.hvm_domain.ioreq_server.list,
> > list_entry )
> > {
> > - ioreq_t *p = get_ioreq(s, v);
> > + struct hvm_ioreq_vcpu *sv;
> >
> > - /* NB. Optimised for common case (p->state ==
> STATE_IOREQ_NONE). */
> > - while ( p->state != STATE_IOREQ_NONE )
> > + list_for_each_entry ( sv,
> > + &s->ioreq_vcpu_list,
> > + list_entry )
> > {
> > - switch ( p->state )
> > + if ( sv->vcpu == v )
> > {
> > - case STATE_IORESP_READY: /* IORESP_READY -> NONE */
> > - rmb(); /* see IORESP_READY /then/ read contents of ioreq */
> > - hvm_io_assist(p);
> > - break;
> > - case STATE_IOREQ_READY: /* IOREQ_{READY,INPROCESS} ->
> IORESP_READY */
> > - case STATE_IOREQ_INPROCESS:
> > - wait_on_xen_event_channel(p->vp_eport,
> > - (p->state != STATE_IOREQ_READY) &&
> > - (p->state != STATE_IOREQ_INPROCESS));
> > - break;
> > - default:
> > - gdprintk(XENLOG_ERR, "Weird HVM iorequest state %d.\n", p-
> >state);
> > - domain_crash(d);
> > - return; /* bail */
>
> By pulling this return into hvm_wait_for_io(), you now continue through
> hvm_do_resume() even after deciding to crash the domain, which has
> changed the error behaviour and doesn't look like it will go well with
> hvm_inject_trap() out of context below. You probably want a boolean
> return from hvm_wait_for_io().
Yes, that's a good point. I do need to bail.
>
> > + ioreq_t *p = get_ioreq(s, v);
> > +
> > + hvm_wait_for_io(sv, p);
>
> You presumably want to break at this point, or you will pointlessly
> continue the list_for_each_entry() loop. You can also get away with
> folding these 3 lines together and doing away with 'p'.
Yes, the inner loop should be exited.
Thanks,
Paul
>
> ~Andrew
>
> > }
> > }
> > }
> > @@ -2545,35 +2560,58 @@ bool_t
> hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
> > {
> > struct vcpu *curr = current;
> > struct domain *d = curr->domain;
> > - ioreq_t *p;
> > + struct hvm_ioreq_vcpu *sv;
> >
> > if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> > return 0; /* implicitly bins the i/o operation */
> >
> > - p = get_ioreq(s, curr);
> > -
> > - if ( unlikely(p->state != STATE_IOREQ_NONE) )
> > + list_for_each_entry ( sv,
> > + &s->ioreq_vcpu_list,
> > + list_entry )
> > {
> > - /* This indicates a bug in the device model. Crash the domain. */
> > - gdprintk(XENLOG_ERR, "Device model set bad IO state %d.\n", p-
> >state);
> > - domain_crash(d);
> > - return 0;
> > - }
> > + if ( sv->vcpu == curr )
> > + {
> > + evtchn_port_t port = sv->ioreq_evtchn;
> > + ioreq_t *p = get_ioreq(s, curr);
> >
> > - proto_p->state = STATE_IOREQ_NONE;
> > - proto_p->vp_eport = p->vp_eport;
> > - *p = *proto_p;
> > + if ( unlikely(p->state != STATE_IOREQ_NONE) )
> > + {
> > + gdprintk(XENLOG_ERR,
> > + "Device model set bad IO state %d.\n",
> > + p->state);
> > + goto crash;
> > + }
> >
> > - prepare_wait_on_xen_event_channel(p->vp_eport);
> > + if ( unlikely(p->vp_eport != port) )
> > + {
> > + gdprintk(XENLOG_ERR,
> > + "Device model set bad event channel %d.\n",
> > + p->vp_eport);
> > + goto crash;
> > + }
> >
> > - /*
> > - * Following happens /after/ blocking and setting up ioreq contents.
> > - * prepare_wait_on_xen_event_channel() is an implicit barrier.
> > - */
> > - p->state = STATE_IOREQ_READY;
> > - notify_via_xen_event_channel(d, p->vp_eport);
> > + proto_p->state = STATE_IOREQ_NONE;
> > + proto_p->vp_eport = port;
> > + *p = *proto_p;
> > +
> > + prepare_wait_on_xen_event_channel(port);
> > +
> > + /*
> > + * Following happens /after/ blocking and setting up ioreq
> > + * contents. prepare_wait_on_xen_event_channel() is an implicit
> > + * barrier.
> > + */
> > + p->state = STATE_IOREQ_READY;
> > + notify_via_xen_event_channel(d, port);
> > + break;
> > + }
> > + }
> >
> > return 1;
> > +
> > + crash:
> > + domain_crash(d);
> > + return 0;
> > }
> >
> > bool_t hvm_send_assist_req(ioreq_t *p)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-14 13:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-14 10:21 [PATCH] Fix ioreq-server event channel vulnerability Paul Durrant
2014-07-14 13:53 ` Andrew Cooper
2014-07-14 13:58 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.