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

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

On 08/07/14 00:18, 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@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

This patch uses the same v->arch.smap_check_policy variable as the
previous patch, meaning its value is unconditionally
SMAP_CHECK_HONOR_CPL_AC other than the short window while the runstate
area is being updated.

As a result, there appears to be no way for this to cause working
updates to a system time living in guest userspace.

~Andrew

> ---
>  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
> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.
> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*