Re: [Xen-devel] [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> Sent: Wednesday, July 09, 2014 12:14 AM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; jbeulich@xxxxxxxx; tim@xxxxxxx;
> linux@xxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> On Tue, Jul 08, 2014 at 07:18:18AM +0800, Feng Wu wrote:
> > In this patch, we add a new hypervisor which allows guests to enable
> > SMAP check when Xen is updating the secondary system time for guest.
> > The SMAP check for this update is disabled by default.
> >
> > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/domain.c        |  6 ++++++
> >  xen/arch/x86/hvm/hvm.c       |  2 ++
> >  xen/arch/x86/time.c          | 12 +++++++++++-
> >  xen/include/asm-x86/domain.h |  9 ++++++++-
> >  xen/include/public/vcpu.h    | 10 ++++++++++
> >  5 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index b0c8810..a787c46 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
> >          break;
> >      }
> >
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> > +    {
> > +        v->arch.enable_smap_check = 1;
> > +        break;
> > +    }
> > +
> >      case VCPUOP_get_physid:
> >      {
> >          struct vcpu_get_physid cpu_id;
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef2411c..a922711 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = do_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = compat_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index a4e1656..338381e 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -821,7 +821,7 @@ static void __update_vcpu_system_time(struct vcpu
> *v, int force)
> >          v->arch.pv_vcpu.pending_system_time = _u;
> >  }
> >
> > -bool_t update_secondary_system_time(const struct vcpu *v,
> > +bool_t update_secondary_system_time(struct vcpu *v,
> >                                      struct vcpu_time_info *u)
> >  {
> >      XEN_GUEST_HANDLE(vcpu_time_info_t) user_u =
> v->arch.time_info_guest;
> > @@ -829,9 +829,16 @@ bool_t update_secondary_system_time(const
> struct vcpu *v,
> >      if ( guest_handle_is_null(user_u) )
> >          return 1;
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> > +
> >      /* 1. Update userspace version. */
> >      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> > +    {
> > +        if ( v->arch.enable_smap_check )
> > +            v->arch.smap_check_policy =
> SMAP_CHECK_HONOR_CPL_AC;
> >          return 0;
> > +    }
> >      wmb();
> >      /* 2. Update all other userspace fields. */
> >      __copy_to_guest(user_u, u, 1);
> > @@ -840,6 +847,9 @@ bool_t update_secondary_system_time(const struct
> vcpu *v,
> >      u->version = version_update_end(u->version);
> >      __copy_field_to_guest(user_u, u, version);
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
> > +
> >      return 1;
> >  }
> >
> > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> > index d7cac4f..051fed0 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -455,6 +455,13 @@ struct arch_vcpu
> >       *     SMAP_CHECK_DISABLED     - disable the check
> >       */
> >      uint8_t smap_check_policy;
> > +
> > +    /*
> > +     * This is a flag to indicate that whether the guest wants to enable
> > +     * SMAP check when Xen is updating the secondary system time from
> it.
> > +     */
> > +    bool_t enable_smap_check;
> > +    uint16_t pad;
> >  } __cacheline_aligned;
> >
> >  #define SMAP_CHECK_HONOR_CPL_AC  0
> > @@ -466,7 +473,7 @@ struct arch_vcpu
> >  #define hvm_svm         hvm_vcpu.u.svm
> >
> >  bool_t update_runstate_area(struct vcpu *);
> > -bool_t update_secondary_system_time(const struct vcpu *,
> > +bool_t update_secondary_system_time(struct vcpu *,
> >                                      struct vcpu_time_info *);
> >
> >  void vcpu_show_execution_state(struct vcpu *);
> > diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> > index e888daf..d348395 100644
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >  typedef struct vcpu_register_time_memory_area
> vcpu_register_time_memory_area_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +/*
> > + * Flags to tell Xen whether we need to do the SMAP check when updating
> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> > + * memory location for the secondary copy of the vcpu time may be mapped
> > + * into userspace by guests intendedly, we let the guest to determine
> 
> What is 'intendedly'?
> 
> > + * whether the check is needed. The default behavior of hypevisor is
> > + * not doing the check.
> 
> Under what conditions do we want the guest to do the check? What is
> the impact (performance) if we do the check?
> 
> Why don't we want by default do the check?

If SMAP is enabled, supervisor mode accesses to user-accessible pages are not
allowed when CPL=0 & EFLAGS.AC=0. 

- If the address passed to hypervisor is in guest kernel space, we need to do
the check. If the check fails, it means some abnormal happened.
- If the address passed to hypervisor is in guest user space, which means guest
wants hypervisor to put the information to its user-space address. In this case,
We shouldn't do the check, or a SMAP violation will be triggered and this is not
what the guest wants.

Thanks,
Feng

> 
> > + */
> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> > +
> >  #endif /* __XEN_PUBLIC_VCPU_H__ */
> >
> >  /*
> > --
> > 1.8.3.1
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel