Re: [PATCH] Fix ioreq-server event channel vulnerability

[Andrew Cooper <andrew.cooper3@citrix.com>]

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)