[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxxxx> > Reported-by: Wen Congyang <wency@xxxxxxxxxxxxxx> > Cc: Keir Fraser <keir@xxxxxxx> > Cc: Jan Beulich <jbeulich@xxxxxxxx> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > 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) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |