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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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"?

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

Jan


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