Re: [Xen-devel] [PATCH 5/6] x86/hvm: Handle x2apic MSRs the new guest_{rd, wr}msr() infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.18 at 18:35, <andrew.cooper3@xxxxxxxxxx> wrote:

"in" missing from subject?

> Dispatch from the guest_{rd,wr}msr() functions.  The read side should be safe
> outside of current context, but the write side is definitely not.  As the
> toolstack has plausible reason to access the APIC registers via this interface

... has no plausible reason ... ?

> @@ -983,49 +982,52 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int 
> offset)
>      return X86EMUL_OKAY;
>  }
>  
> -int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t 
> msr_content)
> +int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>      uint32_t offset = (msr - MSR_IA32_APICBASE_MSR) << 4;
>  
> +    /* The timer handling at least is unsafe outside of current context. */
> +    ASSERT(v == current);
> +
>      if ( !vlapic_x2apic_mode(vlapic) )
> -        return X86EMUL_UNHANDLEABLE;
> +        goto gp_fault;
>  
>      switch ( offset )
>      {
>      case APIC_TASKPRI:
>          if ( msr_content & ~APIC_TPRI_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_SPIV:
>          if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
>                               (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
>                                ? APIC_SPIV_DIRECTED_EOI : 0)) )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTT:
>          if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTTHMR:
>      case APIC_LVTPC:
>      case APIC_CMCI:
>          if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVT0:
>      case APIC_LVT1:
>          if ( msr_content & ~LINT_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTERR:
>          if ( msr_content & ~LVT_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_TMICT:
> @@ -1033,20 +1035,20 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int 
> msr, uint64_t msr_content)
>  
>      case APIC_TDCR:
>          if ( msr_content & ~APIC_TDR_DIV_1 )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_ICR:
>          if ( (uint32_t)msr_content & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
>                                         APIC_DEST_MASK | APIC_INT_ASSERT |
>                                         APIC_INT_LEVELTRIG | APIC_SHORT_MASK) 
> )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>          break;
>  
>      case APIC_SELF_IPI:
>          if ( msr_content & ~APIC_VECTOR_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          offset = APIC_ICR;
>          msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
>          break;
> @@ -1054,13 +1056,18 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int 
> msr, uint64_t msr_content)
>      case APIC_EOI:
>      case APIC_ESR:
>          if ( msr_content )
> +        {
>      default:
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
> +        }
>      }
>  
>      vlapic_reg_write(v, offset, msr_content);
>  
>      return X86EMUL_OKAY;
> +
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

There's really no good reason to use goto all over the place here
when all that's at the label is a single return statement. I was
actually hoping to do conversions the other way around in some
other code.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,6 +139,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> +    const struct vcpu *curr = current;
>      const struct domain *d = v->domain;
>      const struct cpuid_policy *cp = d->arch.cpuid;
>      const struct msr_domain_policy *dp = d->arch.msr;
> @@ -175,6 +176,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;

"v != curr" is quite certainly not a reason to raise #GP.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel