Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

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

On 04/07/14 09:34, Dongxiao Xu wrote:
> Add a generic resource access hypercall for tool stack or other
> components, e.g., accessing MSR, port I/O, etc.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

This still permits a user of the hypercalls to play with EFER or
SYSENTER_EIP, which obviously is a very bad thing.

There needs to be a whitelist of permitted MSRs which can be accessed.

~Andrew

> ---
>  xen/arch/x86/Makefile             |    1 +
>  xen/arch/x86/platform_hypercall.c |   39 ++++++++++++
>  xen/arch/x86/resource.c           |  119 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/resource.h    |   40 +++++++++++++
>  xen/include/public/platform.h     |   24 ++++++++
>  xen/include/xlat.lst              |    1 +
>  6 files changed, 224 insertions(+)
>  create mode 100644 xen/arch/x86/resource.c
>  create mode 100644 xen/include/asm-x86/resource.h
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 6c90b1b..e0cee24 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -59,6 +59,7 @@ obj-y += crash.o
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += xstate.o
> +obj-y += resource.o
>  
>  obj-$(crash_debug) += gdbstub.o
>  
> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> index 2162811..da3d6c4 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -32,6 +32,7 @@
>  #include <asm/setup.h>
>  #include "cpu/mtrr/mtrr.h"
>  #include <xsm/xsm.h>
> +#include <asm/resource.h>
>  
>  #ifndef COMPAT
>  typedef long ret_t;
> @@ -601,6 +602,44 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>      }
>      break;
>  
> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access info;
> +
> +        info.nr = op->u.resource_op.nr;
> +        info.type = op->u.resource_op.type;
> +        info.data = xmalloc_array(xenpf_resource_data_t, info.nr);
> +        if ( !info.data )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        if ( copy_from_guest(info.data, op->u.resource_op.data, info.nr) )
> +        {
> +            xfree(info.data);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        ret = resource_access_helper(&info);
> +        if ( ret )
> +        {
> +            xfree(info.data);
> +            break;
> +        }
> +
> +        if ( copy_to_guest(op->u.resource_op.data, info.data, info.nr) )
> +        {
> +            xfree(info.data);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        xfree(info.data);
> +    }
> +    break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/arch/x86/resource.c b/xen/arch/x86/resource.c
> new file mode 100644
> index 0000000..cc548cd
> --- /dev/null
> +++ b/xen/arch/x86/resource.c
> @@ -0,0 +1,119 @@
> +/*
> + * resource.c: Helpers for Dom0 to access system resource
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <xen/types.h>
> +#include <xen/domain.h>
> +#include <xen/guest_access.h>
> +#include <public/platform.h>
> +#include <asm/msr.h>
> +#include <asm/event.h>
> +#include <asm/resource.h>
> +
> +static int resource_access_one(uint32_t type, uint32_t cmd,
> +                                uint64_t idx, uint64_t *val)
> +{
> +    int ret = 0;
> +
> +    switch ( type )
> +    {
> +    case XEN_RESOURCE_TYPE_MSR:
> +        if ( cmd == XEN_RESOURCE_OP_READ )
> +            ret = rdmsr_safe(idx, *val);
> +        else if ( cmd == XEN_RESOURCE_OP_WRITE )
> +            ret = wrmsr_safe(idx, *val);
> +        break;
> +
> +    default:
> +        gdprintk(XENLOG_WARNING, "unsupported resource type: %d\n", type);
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void resource_access_multi(void *param)
> +{
> +    struct xen_resource_access *info = param;
> +    unsigned int i;
> +    int ret = 0;
> +
> +    for ( i = 0; i < info->nr; i++ )
> +    {
> +        if ( !is_idle_vcpu(current) && hypercall_preempt_check() )
> +        {
> +            ret = -ERESTART;
> +            break;
> +        }
> +        ret = resource_access_one(info->type, info->data[i].cmd,
> +                                  info->data[i].idx, &info->data[i].val);
> +        if ( ret )
> +            break;
> +    }
> +
> +    info->ret = ret;
> +}
> +
> +int resource_access_helper(struct xen_resource_access *info)
> +{
> +    struct xen_resource_access iter;
> +    unsigned int i, last_cpu = ~0;
> +
> +    iter.ret = 0;
> +    iter.nr = 0;
> +    iter.type = info->type;
> +    iter.data = info->data;
> +
> +    for ( i = 0; i < info->nr; i++ )
> +    {
> +        if ( iter.nr && info->data[i].cpu != last_cpu )
> +        {
> +            if ( last_cpu == smp_processor_id() )
> +                resource_access_multi(&iter);
> +            else
> +                /* Set wait=1 to ensure the access order  */
> +                on_selected_cpus(cpumask_of(last_cpu),
> +                                 resource_access_multi, &iter, 1);
> +
> +            if ( iter.ret )
> +                return iter.ret;
> +
> +            iter.nr = 0;
> +            iter.data = &info->data[i];
> +        }
> +
> +        last_cpu = info->data[i].cpu;
> +        iter.nr++;
> +    }
> +
> +    if ( last_cpu == smp_processor_id() )
> +        resource_access_multi(&iter);
> +    else
> +        on_selected_cpus(cpumask_of(last_cpu),
> +                         resource_access_multi, &iter, 1);
> +
> +    return iter.ret;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-x86/resource.h b/xen/include/asm-x86/resource.h
> new file mode 100644
> index 0000000..74b46be
> --- /dev/null
> +++ b/xen/include/asm-x86/resource.h
> @@ -0,0 +1,40 @@
> +/*
> + * resource.h: Helpers for Dom0 to access system resource
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#ifndef __ASM_RESOURCE_H__
> +#define __ASM_RESOURCE_H__
> +
> +#include <public/platform.h>
> +
> +struct xen_resource_access {
> +    int32_t ret;
> +    uint32_t nr;
> +    uint32_t type;
> +    xenpf_resource_data_t *data;
> +};
> +
> +int resource_access_helper(struct xen_resource_access *info);
> +
> +#endif /* __ASM_RESOURCE_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> index 053b9fa..eafdc8a 100644
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -527,6 +527,29 @@ struct xenpf_core_parking {
>  typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
> +#define XENPF_resource_op   61
> +
> +#define XEN_RESOURCE_OP_READ  0
> +#define XEN_RESOURCE_OP_WRITE 1
> +
> +#define XEN_RESOURCE_TYPE_MSR  0
> +
> +struct xenpf_resource_data {
> +    uint32_t cmd;       /* XEN_RESOURCE_OP_* */
> +    uint32_t cpu;
> +    uint64_t idx;
> +    uint64_t val;
> +};
> +typedef struct xenpf_resource_data xenpf_resource_data_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t);
> +struct xenpf_resource_op {
> +    uint32_t nr;
> +    uint32_t type;      /* XEN_RESOURCE_TYPE_* */
> +    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
> +};
> +typedef struct xenpf_resource_op xenpf_resource_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t);
> +
>  /*
>   * ` enum neg_errnoval
>   * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
> @@ -553,6 +576,7 @@ struct xen_platform_op {
>          struct xenpf_cpu_hotadd        cpu_add;
>          struct xenpf_mem_hotadd        mem_add;
>          struct xenpf_core_parking      core_parking;
> +        struct xenpf_resource_op       resource_op;
>          uint8_t                        pad[128];
>      } u;
>  };
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 9a35dd7..06ed7b9 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -88,6 +88,7 @@
>  ?	xenpf_enter_acpi_sleep		platform.h
>  ?	xenpf_pcpuinfo			platform.h
>  ?	xenpf_pcpu_version		platform.h
> +?	xenpf_resource_op		platform.h
>  !	sched_poll			sched.h
>  ?	sched_remote_shutdown		sched.h
>  ?	sched_shutdown			sched.h