[RFC PATCH v2 for-4.5] x86/hvm: Further restrict access to x2apic MSRs

[RFC PATCH v2 for-4.5] 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.  All reserved MSRs in this region
are architecturally required to raise #GP faults upon access.

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.

This patch undoes the patch to XSA-108, returning the primary bounds check to
the entire specified range.  It changes hvm_x2apic_msr_read() to a whitelist
approach, which avoids the vulnerability, and provides a more architecturally
accurate emulation of the reserved MSRs (which would previously read as 0
rather than fault).

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>

---

This is RFC because I have not yet functionally tested it, and I would
appreciate feedback on the approach.
---
 xen/arch/x86/hvm/hvm.c    |    4 ++--
 xen/arch/x86/hvm/vlapic.c |   27 +++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f0e1edc..559b769 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4355,7 +4355,7 @@ 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 + 0x3ff:
         if ( hvm_x2apic_msr_read(v, msr, msr_content) )
             goto gp_fault;
         break;
@@ -4482,7 +4482,7 @@ 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 + 0x3ff:
         if ( hvm_x2apic_msr_write(v, msr, msr_content) )
             goto gp_fault;
         break;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 089d13f..b33d24d 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -649,19 +649,38 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
     if ( !vlapic_x2apic_mode(vlapic) )
         return X86EMUL_UNHANDLEABLE;
 
-    vlapic_read_aligned(vlapic, offset, &low);
     switch ( offset )
     {
     case APIC_ICR:
         vlapic_read_aligned(vlapic, APIC_ICR2, &high);
+        /* Fallthrough. */
+    case APIC_ID:
+    case APIC_LVR:
+    case APIC_TASKPRI:
+    case APIC_PROCPRI:
+    case APIC_LDR:
+    case APIC_SPIV:
+    case APIC_ISR ... APIC_ISR + 0x70:
+    case APIC_TMR ... APIC_TMR + 0x70:
+    case APIC_IRR ... APIC_IRR + 0x70:
+    case APIC_ESR:
+    case APIC_CMCI:
+    case APIC_LVTT:
+    case APIC_LVTTHMR:
+    case APIC_LVTPC:
+    case APIC_LVT0:
+    case APIC_LVT1:
+    case APIC_LVTERR:
+    case APIC_TMICT:
+    case APIC_TMCCT:
+    case APIC_TDCR:
         break;
 
-    case APIC_EOI:
-    case APIC_ICR2:
-    case APIC_SELF_IPI:
+    default:
         return X86EMUL_UNHANDLEABLE;
     }
 
+    vlapic_read_aligned(vlapic, offset, &low);
     *msr_content = (((uint64_t)high) << 32) | low;
 
     return X86EMUL_OKAY;
-- 
1.7.10.4

Re: [RFC PATCH v2 for-4.5] x86/hvm: Further restrict access to x2apic MSRs

[Jan Beulich]

>>> On 17.10.14 at 16:49, <andrew.cooper3@citrix.com> wrote:
> The x2apic specification reserves the entire MSR range 0x800-0xbff, while only
> the first 0x3f MSRs have defined purposes.  All reserved MSRs in this region
> are architecturally required to raise #GP faults upon access.
> 
> 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

0xa00..0xa02

> 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.
> 
> This patch undoes the patch to XSA-108, returning the primary bounds check to

"undoes the patch to XSA-108"?

> the entire specified range.  It changes hvm_x2apic_msr_read() to a whitelist
> approach, which avoids the vulnerability, and provides a more 
> architecturally
> accurate emulation of the reserved MSRs (which would previously read as 0
> rather than fault).

Mention that hvm_x2apic_msr_write() already uses a white list
approach and hence doesn't need changing?

> This is RFC because I have not yet functionally tested it, and I would
> appreciate feedback on the approach.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one minor change suggestion:

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -649,19 +649,38 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
>      if ( !vlapic_x2apic_mode(vlapic) )
>          return X86EMUL_UNHANDLEABLE;
>  
> -    vlapic_read_aligned(vlapic, offset, &low);
>      switch ( offset )
>      {
>      case APIC_ICR:
>          vlapic_read_aligned(vlapic, APIC_ICR2, &high);
> +        /* Fallthrough. */
> +    case APIC_ID:
> +    case APIC_LVR:
> +    case APIC_TASKPRI:
> +    case APIC_PROCPRI:
> +    case APIC_LDR:
> +    case APIC_SPIV:
> +    case APIC_ISR ... APIC_ISR + 0x70:
> +    case APIC_TMR ... APIC_TMR + 0x70:
> +    case APIC_IRR ... APIC_IRR + 0x70:
> +    case APIC_ESR:
> +    case APIC_CMCI:
> +    case APIC_LVTT:
> +    case APIC_LVTTHMR:
> +    case APIC_LVTPC:
> +    case APIC_LVT0:
> +    case APIC_LVT1:
> +    case APIC_LVTERR:
> +    case APIC_TMICT:
> +    case APIC_TMCCT:
> +    case APIC_TDCR:
>          break;
>  
> -    case APIC_EOI:
> -    case APIC_ICR2:
> -    case APIC_SELF_IPI:
> +    default:
>          return X86EMUL_UNHANDLEABLE;
>      }
>  
> +    vlapic_read_aligned(vlapic, offset, &low);

If you move this up into the switch statement, the compiler will have
a chance to warn about "low" being uninitialized for any unintentional
(future) code path falling out through the bottom of the switch
statement. But I'm not insisting on it - if you decide to keep it the
way is it, the R-b stands anyway.

Jan

Re: [RFC PATCH v2 for-4.5] x86/hvm: Further restrict access to x2apic MSRs

[Andrew Cooper]

On 17/10/14 16:08, Jan Beulich wrote:
>>>> On 17.10.14 at 16:49, <andrew.cooper3@citrix.com> wrote:
>> The x2apic specification reserves the entire MSR range 0x800-0xbff, while only
>> the first 0x3f MSRs have defined purposes.  All reserved MSRs in this region
>> are architecturally required to raise #GP faults upon access.
>>
>> 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
> 0xa00..0xa02
>
>> 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.
>>
>> This patch undoes the patch to XSA-108, returning the primary bounds check to
> "undoes the patch to XSA-108"?

I will reword

>
>> the entire specified range.  It changes hvm_x2apic_msr_read() to a whitelist
>> approach, which avoids the vulnerability, and provides a more 
>> architecturally
>> accurate emulation of the reserved MSRs (which would previously read as 0
>> rather than fault).
> Mention that hvm_x2apic_msr_write() already uses a white list
> approach and hence doesn't need changing?

And will add this in as well.

>
>> This is RFC because I have not yet functionally tested it, and I would
>> appreciate feedback on the approach.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one minor change suggestion:
>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -649,19 +649,38 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
>>      if ( !vlapic_x2apic_mode(vlapic) )
>>          return X86EMUL_UNHANDLEABLE;
>>  
>> -    vlapic_read_aligned(vlapic, offset, &low);
>>      switch ( offset )
>>      {
>>      case APIC_ICR:
>>          vlapic_read_aligned(vlapic, APIC_ICR2, &high);
>> +        /* Fallthrough. */
>> +    case APIC_ID:
>> +    case APIC_LVR:
>> +    case APIC_TASKPRI:
>> +    case APIC_PROCPRI:
>> +    case APIC_LDR:
>> +    case APIC_SPIV:
>> +    case APIC_ISR ... APIC_ISR + 0x70:
>> +    case APIC_TMR ... APIC_TMR + 0x70:
>> +    case APIC_IRR ... APIC_IRR + 0x70:
>> +    case APIC_ESR:
>> +    case APIC_CMCI:
>> +    case APIC_LVTT:
>> +    case APIC_LVTTHMR:
>> +    case APIC_LVTPC:
>> +    case APIC_LVT0:
>> +    case APIC_LVT1:
>> +    case APIC_LVTERR:
>> +    case APIC_TMICT:
>> +    case APIC_TMCCT:
>> +    case APIC_TDCR:
>>          break;
>>  
>> -    case APIC_EOI:
>> -    case APIC_ICR2:
>> -    case APIC_SELF_IPI:
>> +    default:
>>          return X86EMUL_UNHANDLEABLE;
>>      }
>>  
>> +    vlapic_read_aligned(vlapic, offset, &low);
> If you move this up into the switch statement, the compiler will have
> a chance to warn about "low" being uninitialized for any unintentional
> (future) code path falling out through the bottom of the switch
> statement. But I'm not insisting on it - if you decide to keep it the
> way is it, the R-b stands anyway.

I will move it in, and repost once XenRT has finished validating the fix.

~Andrew

Re: [RFC PATCH v2 for-4.5] x86/hvm: Further restrict access to x2apic MSRs

[Nakajima, Jun]

On Fri, Oct 17, 2014 at 8:11 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 17/10/14 16:08, Jan Beulich wrote:
>>>>> On 17.10.14 at 16:49, <andrew.cooper3@citrix.com> wrote:
>>> The x2apic specification reserves the entire MSR range 0x800-0xbff, while only
>>> the first 0x3f MSRs have defined purposes.  All reserved MSRs in this region
>>> are architecturally required to raise #GP faults upon access.
>>>
>>> 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
>> 0xa00..0xa02
>>
>>> 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.
>>>
>>> This patch undoes the patch to XSA-108, returning the primary bounds check to
>> "undoes the patch to XSA-108"?
>
> I will reword
>
>>
>>> the entire specified range.  It changes hvm_x2apic_msr_read() to a whitelist
>>> approach, which avoids the vulnerability, and provides a more
>>> architecturally
>>> accurate emulation of the reserved MSRs (which would previously read as 0
>>> rather than fault).
>> Mention that hvm_x2apic_msr_write() already uses a white list
>> approach and hence doesn't need changing?
>
> And will add this in as well.
>
>>
>>> This is RFC because I have not yet functionally tested it, and I would
>>> appreciate feedback on the approach.
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one minor change suggestion:
>>
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -649,19 +649,38 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
>>>      if ( !vlapic_x2apic_mode(vlapic) )
>>>          return X86EMUL_UNHANDLEABLE;
>>>
>>> -    vlapic_read_aligned(vlapic, offset, &low);
>>>      switch ( offset )
>>>      {
>>>      case APIC_ICR:
>>>          vlapic_read_aligned(vlapic, APIC_ICR2, &high);
>>> +        /* Fallthrough. */
>>> +    case APIC_ID:
>>> +    case APIC_LVR:
>>> +    case APIC_TASKPRI:
>>> +    case APIC_PROCPRI:
>>> +    case APIC_LDR:
>>> +    case APIC_SPIV:
>>> +    case APIC_ISR ... APIC_ISR + 0x70:
>>> +    case APIC_TMR ... APIC_TMR + 0x70:
>>> +    case APIC_IRR ... APIC_IRR + 0x70:
>>> +    case APIC_ESR:
>>> +    case APIC_CMCI:
>>> +    case APIC_LVTT:
>>> +    case APIC_LVTTHMR:
>>> +    case APIC_LVTPC:
>>> +    case APIC_LVT0:
>>> +    case APIC_LVT1:
>>> +    case APIC_LVTERR:
>>> +    case APIC_TMICT:
>>> +    case APIC_TMCCT:
>>> +    case APIC_TDCR:
>>>          break;
>>>
>>> -    case APIC_EOI:
>>> -    case APIC_ICR2:
>>> -    case APIC_SELF_IPI:
>>> +    default:
>>>          return X86EMUL_UNHANDLEABLE;
>>>      }
>>>
>>> +    vlapic_read_aligned(vlapic, offset, &low);
>> If you move this up into the switch statement, the compiler will have
>> a chance to warn about "low" being uninitialized for any unintentional
>> (future) code path falling out through the bottom of the switch
>> statement. But I'm not insisting on it - if you decide to keep it the
>> way is it, the R-b stands anyway.
>
> I will move it in, and repost once XenRT has finished validating the fix.
>
> ~Andrew

Assuming that the update reflecting the above be made,

Acked-by: Jun Nakajima <jun.nakajima@intel.com>

-- 
Jun
Intel Open Source Technology Center