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

["Nakajima, Jun" <jun.nakajima@xxxxxxxxx>]

On Fri, Oct 17, 2014 at 8:11 AM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> On 17/10/14 16:08, Jan Beulich wrote:
>>>>> On 17.10.14 at 16:49, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
>> 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@xxxxxxxxx>

-- 
Jun
Intel Open Source Technology Center

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