From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC PATCH v2 for-4.5] x86/hvm: Further restrict access to x2apic MSRs Date: Fri, 17 Oct 2014 16:11:29 +0100 Message-ID: <544131A1.8000402@citrix.com> References: <1413557398-21753-1-git-send-email-andrew.cooper3@citrix.com> <54414D00020000780003FAE6@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54414D00020000780003FAE6@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Kevin Tian , Feng Wu , Matt Wilson , Eddie Dong , Donald Dugger , Xen-devel , Jun Nakajima , Yang Zhang , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 17/10/14 16:08, Jan Beulich wrote: >>>> On 17.10.14 at 16:49, 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 > 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