[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Fix ioreq-server event channel vulnerability
> -----Original Message----- > From: Andrew Cooper > Sent: 14 July 2014 14:53 > To: Paul Durrant; xen-devel@xxxxxxxxxxxxx > 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@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(). 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) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |