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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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