[PATCH] x86/hvm: Further restrict access to x2apic MSRs

[PATCH] x86/hvm: Further restrict access to x2apic MSRs

[Andrew Cooper]

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).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Matt Wilson <msw@linux.com>
CC: Yang Zhang <yang.z.zhang@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: Donald Dugger <donald.d.dugger@intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
---
 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;
 
-- 
1.7.10.4

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

[Matt Wilson]

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@amazon.com>

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Matt Wilson <msw@linux.com>
> CC: Yang Zhang <yang.z.zhang@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Eddie Dong <eddie.dong@intel.com>
> CC: Donald Dugger <donald.d.dugger@intel.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> ---
>  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;
>  

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

[Andrew Cooper]

On 17/10/14 09:54, Matt Wilson wrote:
> 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@amazon.com>

Ah - I had a nagging feeling I had forgotten to mention something in the
commit message.

If I need to respin the patch for any reason, I shall include this.  If
not, perhaps the committer would oblige on my behalf?

~Andrew

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

[Jan Beulich]

>>> On 16.10.14 at 20:04, <andrew.cooper3@citrix.com> wrote:
> 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).

There seems to be stuff missing between "at" and "a"; at least I can't
make sense of the sentence.

> --- 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;
>  

I don't think this is done properly here. The ranges here should simply
be extended back to cover the whole 1k range. Anything more specific
should be contained to vlapic.c. And this "more specific" should likely
include #GP on accesses to register indices 0, 1, 4..7, 9, 0xc, 0xe,
0x29..0x2e, and 0x3a..0x3d. I.e. extend the recent tightening,
perhaps by fully switching to a white list in hvm_x2apic_msr_read()
instead of the current black list approach, as otherwise what Matt
said would still be wrong for all the listed registers.

Jan

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

[Andrew Cooper]

On 17/10/14 10:59, Jan Beulich wrote:
>>>> On 16.10.14 at 20:04, <andrew.cooper3@citrix.com> wrote:
>> 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).
> There seems to be stuff missing between "at" and "a"; at least I can't
> make sense of the sentence.

Oops.  The paragraph read "...unconditionally at a\n#GP fault...".  It
appears that git considered the line as a comment and dropped it from a
resulting message.

>
>> --- 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;
>>  
> I don't think this is done properly here. The ranges here should simply
> be extended back to cover the whole 1k range. Anything more specific
> should be contained to vlapic.c. And this "more specific" should likely
> include #GP on accesses to register indices 0, 1, 4..7, 9, 0xc, 0xe,
> 0x29..0x2e, and 0x3a..0x3d. I.e. extend the recent tightening,
> perhaps by fully switching to a white list in hvm_x2apic_msr_read()
> instead of the current black list approach, as otherwise what Matt
> said would still be wrong for all the listed registers.

Ok - I will see what I can do.

~Andrew