Re: [Xen-devel] [PATCH] x86/hvm: Further restrict access to x2apic MSRs

[Matt Wilson <msw@xxxxxxxxx>]

On Thu, Oct 16, 2014 at 07:04:32PM +0100, Andrew Cooper wrote:
> The x2apic specification reserves the entire MSR range 0x800-0xbff, while only
> the first 0x3f MSRs have defined purposes.
> 
> Xen used to pass this entire range to hvm_x2apic_msr_{read,write}(), but the
> range was restricted somewhat by XSA-108 (c/s 61fdda7ac) to prevent guests
> being able to read pages adjacent to the domheap page backing the vlapic->regs
> array.
> 
> While removing the vulnerability, a side effect of XSA-108 was that the MSR
> range 0x900-0xbff fell through the switch statement and ends up reading the
> hosts x2apic range. This behaviour is a problem in general, but specifically
> it turns out that MSRs 0xa00 and 0xa01 are implemented (but undocumented) on
> certain SandyBridge and IvyBridge systems.
> 
> Experimentally, no operating system in XenServer's test suite (including all
> versions of Windows currently supported by Microsoft) ever peek at these MSRs,
> even on hosts where some of them are implemented.
> 
> Therefore, direct the entire reserved range (0x840-0xbff) unconditionally at a
> side effect of this change is that hvm_x2apic_msr_read() can now no longer
> read beyond the bounds of the vlapic->regs array (which is 1/4 the size of the
> page backing it).

Further, Intel(R) 64 Architecture x2 APIC Specification 2.3.4 states:

   RDMSR and WRMSR operations to reserved addresses in the x2APIC mode
   will raise a GP fault.

Therefore RDMSR returning 0 for MSRs 0x840...0x8ff and not causing a
#GP was also not architecturally correct.

Reviewed-by: Matt Wilson <msw@xxxxxxxxxx>

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Matt Wilson <msw@xxxxxxxxx>
> CC: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Feng Wu <feng.wu@xxxxxxxxx>
> CC: Eddie Dong <eddie.dong@xxxxxxxxx>
> CC: Donald Dugger <donald.d.dugger@xxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f0e1edc..5680292 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4355,8 +4355,9 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t 
> *msr_content)
>          *msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
>          break;
>  
> -    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0xff:
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3f:
>          if ( hvm_x2apic_msr_read(v, msr, msr_content) )
> +    case MSR_IA32_APICBASE_MSR + 0x40 ... MSR_IA32_APICBASE_MSR + 0x3ff:
>              goto gp_fault;
>          break;
>  
> @@ -4482,8 +4483,9 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
> msr_content)
>          vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
>          break;
>  
> -    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0xff:
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3f:
>          if ( hvm_x2apic_msr_write(v, msr, msr_content) )
> +    case MSR_IA32_APICBASE_MSR + 0x40 ... MSR_IA32_APICBASE_MSR + 0x3ff:
>              goto gp_fault;
>          break;
>  

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