|
[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 |