* [Xen-devel] [PATCH v2 0/2] nvmx: implement support for MSR bitmaps @ 2020-01-29 14:45 Roger Pau Monne 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 1/2] " Roger Pau Monne 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs Roger Pau Monne 0 siblings, 2 replies; 6+ messages in thread From: Roger Pau Monne @ 2020-01-29 14:45 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Jun Nakajima, Wei Liu, Andrew Cooper, Roger Pau Monne Hello, Current nested VMX code advertises support for the MSR bitmap feature, yet the implementation isn't done. Previous to this series Xen just maps the nested guest MSR bitmap (as set by L1) and that's it, the L2 guest ends up using the L1 MSR bitmap. This series adds handling of the L2 MSR bitmap and merging with the L1 MSR bitmap and loading it into the nested guest VMCS. Patch #2 makes sure the x2APIC MSR range is always trapped, or else a guest with nested virtualization enabled could manage to access some of the x2APIC MSR registers from the host. Thanks, Roger. Roger Pau Monne (2): nvmx: implement support for MSR bitmaps nvmx: always trap accesses to x2APIC MSRs xen/arch/x86/hvm/vmx/vvmx.c | 73 ++++++++++++++++++++++++++++-- xen/include/asm-x86/hvm/vmx/vvmx.h | 3 +- 2 files changed, 72 insertions(+), 4 deletions(-) -- 2.25.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH v2 1/2] nvmx: implement support for MSR bitmaps 2020-01-29 14:45 [Xen-devel] [PATCH v2 0/2] nvmx: implement support for MSR bitmaps Roger Pau Monne @ 2020-01-29 14:45 ` Roger Pau Monne 2020-02-03 8:05 ` Tian, Kevin 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs Roger Pau Monne 1 sibling, 1 reply; 6+ messages in thread From: Roger Pau Monne @ 2020-01-29 14:45 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Jun Nakajima, Wei Liu, Andrew Cooper, Roger Pau Monne Current implementation of nested VMX has a half baked handling of MSR bitmaps for the L1 VMM: it maps the L1 VMM provided MSR bitmap, but doesn't actually load it into the nested vmcs, and thus the nested guest vmcs ends up using the same MSR bitmap as the L1 VMM. This is wrong as there's no assurance that the set of features enabled for the L1 vmcs are the same that L1 itself is going to use in the nested vmcs, and thus can lead to misconfigurations. For example L1 vmcs can use x2APIC virtualization and virtual interrupt delivery, and thus some x2APIC MSRs won't be trapped so that they can be handled directly by the hardware using virtualization extensions. On the other hand, the nested vmcs created by L1 VMM might not use any of such features, so using a MSR bitmap that doesn't trap accesses to the x2APIC MSRs will be leaking them to the underlying hardware. Fix this by crafting a merged MSR bitmap between the one used by L1 and the nested guest. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- This seems better than what's done currently, but TBH there's a lot of work to be done in nvmx in order to make it functional and secure that I'm not sure whether building on top of the current implementation is something sane to do, or it would be better to start from scratch and re-implement nvmx to just support the minimum required set of VTx features in a sane and safe way. --- Changes since v1: - Split the x2APIC MSR fix into a separate patch. - Move setting MSR_BITMAP vmcs field into load_vvmcs_host_state for virtual vmexit. - Allocate memory with MEMF_no_owner. - Use tabs to align comment of the nestedvmx struct field. --- xen/arch/x86/hvm/vmx/vvmx.c | 63 ++++++++++++++++++++++++++++-- xen/include/asm-x86/hvm/vmx/vvmx.h | 3 +- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 47eee1e5b9..c35b4bab84 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -128,6 +128,16 @@ int nvmx_vcpu_initialise(struct vcpu *v) unmap_domain_page(vw); } + if ( cpu_has_vmx_msr_bitmap ) + { + nvmx->msr_merged = alloc_domheap_page(d, MEMF_no_owner); + if ( !nvmx->msr_merged ) + { + gdprintk(XENLOG_ERR, "nest: allocation for MSR bitmap failed\n"); + return -ENOMEM; + } + } + nvmx->ept.enabled = 0; nvmx->guest_vpid = 0; nvmx->vmxon_region_pa = INVALID_PADDR; @@ -182,6 +192,11 @@ void nvmx_vcpu_destroy(struct vcpu *v) free_domheap_page(v->arch.hvm.vmx.vmwrite_bitmap); v->arch.hvm.vmx.vmwrite_bitmap = NULL; } + if ( nvmx->msr_merged ) + { + free_domheap_page(nvmx->msr_merged); + nvmx->msr_merged = NULL; + } } void nvmx_domain_relinquish_resources(struct domain *d) @@ -548,6 +563,37 @@ unsigned long *_shadow_io_bitmap(struct vcpu *v) return nestedhvm_vcpu_iomap_get(port80, portED); } +static void update_msrbitmap(struct vcpu *v) +{ + struct nestedvmx *nvmx = &vcpu_2_nvmx(v); + struct vmx_msr_bitmap *msr_bitmap; + unsigned int msr; + + ASSERT(__n2_exec_control(v) & CPU_BASED_ACTIVATE_MSR_BITMAP); + + if ( !nvmx->msrbitmap ) + return; + + msr_bitmap = __map_domain_page(nvmx->msr_merged); + + bitmap_or(msr_bitmap->read_low, nvmx->msrbitmap->read_low, + v->arch.hvm.vmx.msr_bitmap->read_low, + sizeof(msr_bitmap->read_low) * 8); + bitmap_or(msr_bitmap->read_high, nvmx->msrbitmap->read_high, + v->arch.hvm.vmx.msr_bitmap->read_high, + sizeof(msr_bitmap->read_high) * 8); + bitmap_or(msr_bitmap->write_low, nvmx->msrbitmap->write_low, + v->arch.hvm.vmx.msr_bitmap->write_low, + sizeof(msr_bitmap->write_low) * 8); + bitmap_or(msr_bitmap->write_high, nvmx->msrbitmap->write_high, + v->arch.hvm.vmx.msr_bitmap->write_high, + sizeof(msr_bitmap->write_high) * 8); + + unmap_domain_page(msr_bitmap); + + __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged)); +} + void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl) { u32 pio_cntrl = (CPU_BASED_ACTIVATE_IO_BITMAP @@ -558,10 +604,15 @@ void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl) shadow_cntrl = __n2_exec_control(v); pio_cntrl &= shadow_cntrl; /* Enforce the removed features */ - shadow_cntrl &= ~(CPU_BASED_ACTIVATE_MSR_BITMAP - | CPU_BASED_ACTIVATE_IO_BITMAP + shadow_cntrl &= ~(CPU_BASED_ACTIVATE_IO_BITMAP | CPU_BASED_UNCOND_IO_EXITING); - shadow_cntrl |= host_cntrl; + /* + * Do NOT enforce the MSR bitmap currently used by L1, as certain hardware + * virtualization features require specific MSR bitmap settings, but + * without the guest also using these same features the bitmap could be + * leaking through unwanted MSR accesses. + */ + shadow_cntrl |= (host_cntrl & ~CPU_BASED_ACTIVATE_MSR_BITMAP); if ( pio_cntrl == CPU_BASED_UNCOND_IO_EXITING ) { /* L1 VMM intercepts all I/O instructions */ shadow_cntrl |= CPU_BASED_UNCOND_IO_EXITING; @@ -584,6 +635,9 @@ void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl) __vmwrite(IO_BITMAP_B, virt_to_maddr(bitmap) + PAGE_SIZE); } + if ( shadow_cntrl & CPU_BASED_ACTIVATE_MSR_BITMAP ) + update_msrbitmap(v); + /* TODO: change L0 intr window to MTF or NMI window */ __vmwrite(CPU_BASED_VM_EXEC_CONTROL, shadow_cntrl); } @@ -1278,6 +1332,9 @@ static void load_vvmcs_host_state(struct vcpu *v) hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset, 0); set_vvmcs(v, VM_ENTRY_INTR_INFO, 0); + + if ( v->arch.hvm.vmx.exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP ) + __vmwrite(MSR_BITMAP, virt_to_maddr(v->arch.hvm.vmx.msr_bitmap)); } static void sync_exception_state(struct vcpu *v) diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h index 6b9c4ae0b2..c8d5600fdd 100644 --- a/xen/include/asm-x86/hvm/vmx/vvmx.h +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h @@ -37,7 +37,8 @@ struct nestedvmx { */ paddr_t vmxon_region_pa; void *iobitmap[2]; /* map (va) of L1 guest I/O bitmap */ - void *msrbitmap; /* map (va) of L1 guest MSR bitmap */ + struct vmx_msr_bitmap *msrbitmap; /* map (va) of L1 guest MSR bitmap */ + struct page_info *msr_merged; /* merged L1 and L1 guest MSR bitmap */ /* deferred nested interrupt */ struct { unsigned long intr_info; -- 2.25.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/2] nvmx: implement support for MSR bitmaps 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 1/2] " Roger Pau Monne @ 2020-02-03 8:05 ` Tian, Kevin 2020-02-03 17:13 ` Roger Pau Monné 0 siblings, 1 reply; 6+ messages in thread From: Tian, Kevin @ 2020-02-03 8:05 UTC (permalink / raw) To: Roger Pau Monne, xen-devel; +Cc: Andrew Cooper, Wei Liu, Nakajima, Jun > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Wednesday, January 29, 2020 10:45 PM > > Current implementation of nested VMX has a half baked handling of MSR > bitmaps for the L1 VMM: it maps the L1 VMM provided MSR bitmap, but > doesn't actually load it into the nested vmcs, and thus the nested > guest vmcs ends up using the same MSR bitmap as the L1 VMM. > > This is wrong as there's no assurance that the set of features enabled > for the L1 vmcs are the same that L1 itself is going to use in the > nested vmcs, and thus can lead to misconfigurations. > > For example L1 vmcs can use x2APIC virtualization and virtual > interrupt delivery, and thus some x2APIC MSRs won't be trapped so that > they can be handled directly by the hardware using virtualization > extensions. On the other hand, the nested vmcs created by L1 VMM might > not use any of such features, so using a MSR bitmap that doesn't trap > accesses to the x2APIC MSRs will be leaking them to the underlying > hardware. > > Fix this by crafting a merged MSR bitmap between the one used by L1 > and the nested guest. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > This seems better than what's done currently, but TBH there's a lot of > work to be done in nvmx in order to make it functional and secure that > I'm not sure whether building on top of the current implementation is > something sane to do, or it would be better to start from scratch and > re-implement nvmx to just support the minimum required set of VTx > features in a sane and safe way. without knowing what "a lot of work" actually means, it's difficult to judge which way is better. But from the listed changes in this series, I think they are reasonable. > --- > Changes since v1: > - Split the x2APIC MSR fix into a separate patch. > - Move setting MSR_BITMAP vmcs field into load_vvmcs_host_state for > virtual vmexit. > - Allocate memory with MEMF_no_owner. > - Use tabs to align comment of the nestedvmx struct field. > --- > xen/arch/x86/hvm/vmx/vvmx.c | 63 ++++++++++++++++++++++++++++-- > xen/include/asm-x86/hvm/vmx/vvmx.h | 3 +- > 2 files changed, 62 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 47eee1e5b9..c35b4bab84 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -128,6 +128,16 @@ int nvmx_vcpu_initialise(struct vcpu *v) > unmap_domain_page(vw); > } > > + if ( cpu_has_vmx_msr_bitmap ) > + { > + nvmx->msr_merged = alloc_domheap_page(d, MEMF_no_owner); > + if ( !nvmx->msr_merged ) > + { > + gdprintk(XENLOG_ERR, "nest: allocation for MSR bitmap failed\n"); > + return -ENOMEM; > + } > + } > + > nvmx->ept.enabled = 0; > nvmx->guest_vpid = 0; > nvmx->vmxon_region_pa = INVALID_PADDR; > @@ -182,6 +192,11 @@ void nvmx_vcpu_destroy(struct vcpu *v) > free_domheap_page(v->arch.hvm.vmx.vmwrite_bitmap); > v->arch.hvm.vmx.vmwrite_bitmap = NULL; > } > + if ( nvmx->msr_merged ) > + { > + free_domheap_page(nvmx->msr_merged); > + nvmx->msr_merged = NULL; > + } > } > > void nvmx_domain_relinquish_resources(struct domain *d) > @@ -548,6 +563,37 @@ unsigned long *_shadow_io_bitmap(struct vcpu *v) > return nestedhvm_vcpu_iomap_get(port80, portED); > } > > +static void update_msrbitmap(struct vcpu *v) > +{ > + struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > + struct vmx_msr_bitmap *msr_bitmap; > + unsigned int msr; > + > + ASSERT(__n2_exec_control(v) & CPU_BASED_ACTIVATE_MSR_BITMAP); what about passing shadow_cntrl and also moving the outer condition check into this function? It is not good to assume that __n2_exec_control always has the same setting as the local variable shadow_cntrl. > + > + if ( !nvmx->msrbitmap ) > + return; > + > + msr_bitmap = __map_domain_page(nvmx->msr_merged); > + > + bitmap_or(msr_bitmap->read_low, nvmx->msrbitmap->read_low, > + v->arch.hvm.vmx.msr_bitmap->read_low, > + sizeof(msr_bitmap->read_low) * 8); > + bitmap_or(msr_bitmap->read_high, nvmx->msrbitmap->read_high, > + v->arch.hvm.vmx.msr_bitmap->read_high, > + sizeof(msr_bitmap->read_high) * 8); > + bitmap_or(msr_bitmap->write_low, nvmx->msrbitmap->write_low, > + v->arch.hvm.vmx.msr_bitmap->write_low, > + sizeof(msr_bitmap->write_low) * 8); > + bitmap_or(msr_bitmap->write_high, nvmx->msrbitmap->write_high, > + v->arch.hvm.vmx.msr_bitmap->write_high, > + sizeof(msr_bitmap->write_high) * 8); > + > + unmap_domain_page(msr_bitmap); > + > + __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged)); > +} > + > void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl) > { > u32 pio_cntrl = (CPU_BASED_ACTIVATE_IO_BITMAP > @@ -558,10 +604,15 @@ void nvmx_update_exec_control(struct vcpu *v, > u32 host_cntrl) > shadow_cntrl = __n2_exec_control(v); > pio_cntrl &= shadow_cntrl; > /* Enforce the removed features */ > - shadow_cntrl &= ~(CPU_BASED_ACTIVATE_MSR_BITMAP > - | CPU_BASED_ACTIVATE_IO_BITMAP > + shadow_cntrl &= ~(CPU_BASED_ACTIVATE_IO_BITMAP > | CPU_BASED_UNCOND_IO_EXITING); > - shadow_cntrl |= host_cntrl; > + /* > + * Do NOT enforce the MSR bitmap currently used by L1, as certain > hardware > + * virtualization features require specific MSR bitmap settings, but > + * without the guest also using these same features the bitmap could be > + * leaking through unwanted MSR accesses. > + */ > + shadow_cntrl |= (host_cntrl & ~CPU_BASED_ACTIVATE_MSR_BITMAP); what about msr bitmap is disabled in host_cntrl? We'd better use AND-ed value from both shadow/host_cntrl for this bit, instead of assuming the policy of current Xen version which enables msr bitmap by default. > if ( pio_cntrl == CPU_BASED_UNCOND_IO_EXITING ) { > /* L1 VMM intercepts all I/O instructions */ > shadow_cntrl |= CPU_BASED_UNCOND_IO_EXITING; > @@ -584,6 +635,9 @@ void nvmx_update_exec_control(struct vcpu *v, u32 > host_cntrl) > __vmwrite(IO_BITMAP_B, virt_to_maddr(bitmap) + PAGE_SIZE); > } > > + if ( shadow_cntrl & CPU_BASED_ACTIVATE_MSR_BITMAP ) > + update_msrbitmap(v); > + > /* TODO: change L0 intr window to MTF or NMI window */ > __vmwrite(CPU_BASED_VM_EXEC_CONTROL, shadow_cntrl); > } > @@ -1278,6 +1332,9 @@ static void load_vvmcs_host_state(struct vcpu *v) > hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset, 0); > > set_vvmcs(v, VM_ENTRY_INTR_INFO, 0); > + > + if ( v->arch.hvm.vmx.exec_control & > CPU_BASED_ACTIVATE_MSR_BITMAP ) > + __vmwrite(MSR_BITMAP, virt_to_maddr(v- > >arch.hvm.vmx.msr_bitmap)); > } > > static void sync_exception_state(struct vcpu *v) > diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm- > x86/hvm/vmx/vvmx.h > index 6b9c4ae0b2..c8d5600fdd 100644 > --- a/xen/include/asm-x86/hvm/vmx/vvmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h > @@ -37,7 +37,8 @@ struct nestedvmx { > */ > paddr_t vmxon_region_pa; > void *iobitmap[2]; /* map (va) of L1 guest I/O bitmap */ > - void *msrbitmap; /* map (va) of L1 guest MSR bitmap > */ > + struct vmx_msr_bitmap *msrbitmap; /* map (va) of L1 guest MSR > bitmap */ > + struct page_info *msr_merged; /* merged L1 and L1 guest MSR > bitmap */ L1 and L2 > /* deferred nested interrupt */ > struct { > unsigned long intr_info; > -- > 2.25.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2 1/2] nvmx: implement support for MSR bitmaps 2020-02-03 8:05 ` Tian, Kevin @ 2020-02-03 17:13 ` Roger Pau Monné 0 siblings, 0 replies; 6+ messages in thread From: Roger Pau Monné @ 2020-02-03 17:13 UTC (permalink / raw) To: Tian, Kevin; +Cc: xen-devel, Wei Liu, Nakajima, Jun, Andrew Cooper On Mon, Feb 03, 2020 at 08:05:48AM +0000, Tian, Kevin wrote: > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: Wednesday, January 29, 2020 10:45 PM > > > > Current implementation of nested VMX has a half baked handling of MSR > > bitmaps for the L1 VMM: it maps the L1 VMM provided MSR bitmap, but > > doesn't actually load it into the nested vmcs, and thus the nested > > guest vmcs ends up using the same MSR bitmap as the L1 VMM. > > > > This is wrong as there's no assurance that the set of features enabled > > for the L1 vmcs are the same that L1 itself is going to use in the > > nested vmcs, and thus can lead to misconfigurations. > > > > For example L1 vmcs can use x2APIC virtualization and virtual > > interrupt delivery, and thus some x2APIC MSRs won't be trapped so that > > they can be handled directly by the hardware using virtualization > > extensions. On the other hand, the nested vmcs created by L1 VMM might > > not use any of such features, so using a MSR bitmap that doesn't trap > > accesses to the x2APIC MSRs will be leaking them to the underlying > > hardware. > > > > Fix this by crafting a merged MSR bitmap between the one used by L1 > > and the nested guest. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > This seems better than what's done currently, but TBH there's a lot of > > work to be done in nvmx in order to make it functional and secure that > > I'm not sure whether building on top of the current implementation is > > something sane to do, or it would be better to start from scratch and > > re-implement nvmx to just support the minimum required set of VTx > > features in a sane and safe way. > > without knowing what "a lot of work" actually means, it's difficult to > judge which way is better. But from the listed changes in this series, > I think they are reasonable. > > > --- > > Changes since v1: > > - Split the x2APIC MSR fix into a separate patch. > > - Move setting MSR_BITMAP vmcs field into load_vvmcs_host_state for > > virtual vmexit. > > - Allocate memory with MEMF_no_owner. > > - Use tabs to align comment of the nestedvmx struct field. > > --- > > xen/arch/x86/hvm/vmx/vvmx.c | 63 ++++++++++++++++++++++++++++-- > > xen/include/asm-x86/hvm/vmx/vvmx.h | 3 +- > > 2 files changed, 62 insertions(+), 4 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > > index 47eee1e5b9..c35b4bab84 100644 > > --- a/xen/arch/x86/hvm/vmx/vvmx.c > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > > @@ -128,6 +128,16 @@ int nvmx_vcpu_initialise(struct vcpu *v) > > unmap_domain_page(vw); > > } > > > > + if ( cpu_has_vmx_msr_bitmap ) > > + { > > + nvmx->msr_merged = alloc_domheap_page(d, MEMF_no_owner); > > + if ( !nvmx->msr_merged ) > > + { > > + gdprintk(XENLOG_ERR, "nest: allocation for MSR bitmap failed\n"); > > + return -ENOMEM; > > + } > > + } > > + > > nvmx->ept.enabled = 0; > > nvmx->guest_vpid = 0; > > nvmx->vmxon_region_pa = INVALID_PADDR; > > @@ -182,6 +192,11 @@ void nvmx_vcpu_destroy(struct vcpu *v) > > free_domheap_page(v->arch.hvm.vmx.vmwrite_bitmap); > > v->arch.hvm.vmx.vmwrite_bitmap = NULL; > > } > > + if ( nvmx->msr_merged ) > > + { > > + free_domheap_page(nvmx->msr_merged); > > + nvmx->msr_merged = NULL; > > + } > > } > > > > void nvmx_domain_relinquish_resources(struct domain *d) > > @@ -548,6 +563,37 @@ unsigned long *_shadow_io_bitmap(struct vcpu *v) > > return nestedhvm_vcpu_iomap_get(port80, portED); > > } > > > > +static void update_msrbitmap(struct vcpu *v) > > +{ > > + struct nestedvmx *nvmx = &vcpu_2_nvmx(v); > > + struct vmx_msr_bitmap *msr_bitmap; > > + unsigned int msr; > > + > > + ASSERT(__n2_exec_control(v) & CPU_BASED_ACTIVATE_MSR_BITMAP); > > what about passing shadow_cntrl and also moving the outer > condition check into this function? It is not good to assume > that __n2_exec_control always has the same setting as the > local variable shadow_cntrl. > > > + > > + if ( !nvmx->msrbitmap ) > > + return; > > + > > + msr_bitmap = __map_domain_page(nvmx->msr_merged); > > + > > + bitmap_or(msr_bitmap->read_low, nvmx->msrbitmap->read_low, > > + v->arch.hvm.vmx.msr_bitmap->read_low, > > + sizeof(msr_bitmap->read_low) * 8); > > + bitmap_or(msr_bitmap->read_high, nvmx->msrbitmap->read_high, > > + v->arch.hvm.vmx.msr_bitmap->read_high, > > + sizeof(msr_bitmap->read_high) * 8); > > + bitmap_or(msr_bitmap->write_low, nvmx->msrbitmap->write_low, > > + v->arch.hvm.vmx.msr_bitmap->write_low, > > + sizeof(msr_bitmap->write_low) * 8); > > + bitmap_or(msr_bitmap->write_high, nvmx->msrbitmap->write_high, > > + v->arch.hvm.vmx.msr_bitmap->write_high, > > + sizeof(msr_bitmap->write_high) * 8); > > + > > + unmap_domain_page(msr_bitmap); > > + > > + __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged)); > > +} > > + > > void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl) > > { > > u32 pio_cntrl = (CPU_BASED_ACTIVATE_IO_BITMAP > > @@ -558,10 +604,15 @@ void nvmx_update_exec_control(struct vcpu *v, > > u32 host_cntrl) > > shadow_cntrl = __n2_exec_control(v); > > pio_cntrl &= shadow_cntrl; > > /* Enforce the removed features */ > > - shadow_cntrl &= ~(CPU_BASED_ACTIVATE_MSR_BITMAP > > - | CPU_BASED_ACTIVATE_IO_BITMAP > > + shadow_cntrl &= ~(CPU_BASED_ACTIVATE_IO_BITMAP > > | CPU_BASED_UNCOND_IO_EXITING); > > - shadow_cntrl |= host_cntrl; > > + /* > > + * Do NOT enforce the MSR bitmap currently used by L1, as certain > > hardware > > + * virtualization features require specific MSR bitmap settings, but > > + * without the guest also using these same features the bitmap could be > > + * leaking through unwanted MSR accesses. > > + */ > > + shadow_cntrl |= (host_cntrl & ~CPU_BASED_ACTIVATE_MSR_BITMAP); > > what about msr bitmap is disabled in host_cntrl? We'd better use AND-ed > value from both shadow/host_cntrl for this bit, instead of assuming the > policy of current Xen version which enables msr bitmap by default. Ack, I've fixed all the above. > > if ( pio_cntrl == CPU_BASED_UNCOND_IO_EXITING ) { > > /* L1 VMM intercepts all I/O instructions */ > > shadow_cntrl |= CPU_BASED_UNCOND_IO_EXITING; > > @@ -584,6 +635,9 @@ void nvmx_update_exec_control(struct vcpu *v, u32 > > host_cntrl) > > __vmwrite(IO_BITMAP_B, virt_to_maddr(bitmap) + PAGE_SIZE); > > } > > > > + if ( shadow_cntrl & CPU_BASED_ACTIVATE_MSR_BITMAP ) > > + update_msrbitmap(v); > > + > > /* TODO: change L0 intr window to MTF or NMI window */ > > __vmwrite(CPU_BASED_VM_EXEC_CONTROL, shadow_cntrl); > > } > > @@ -1278,6 +1332,9 @@ static void load_vvmcs_host_state(struct vcpu *v) > > hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset, 0); > > > > set_vvmcs(v, VM_ENTRY_INTR_INFO, 0); > > + > > + if ( v->arch.hvm.vmx.exec_control & > > CPU_BASED_ACTIVATE_MSR_BITMAP ) > > + __vmwrite(MSR_BITMAP, virt_to_maddr(v- > > >arch.hvm.vmx.msr_bitmap)); > > } > > > > static void sync_exception_state(struct vcpu *v) > > diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm- > > x86/hvm/vmx/vvmx.h > > index 6b9c4ae0b2..c8d5600fdd 100644 > > --- a/xen/include/asm-x86/hvm/vmx/vvmx.h > > +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h > > @@ -37,7 +37,8 @@ struct nestedvmx { > > */ > > paddr_t vmxon_region_pa; > > void *iobitmap[2]; /* map (va) of L1 guest I/O bitmap */ > > - void *msrbitmap; /* map (va) of L1 guest MSR bitmap > > */ > > + struct vmx_msr_bitmap *msrbitmap; /* map (va) of L1 guest MSR > > bitmap */ > > + struct page_info *msr_merged; /* merged L1 and L1 guest MSR > > bitmap */ > > L1 and L2 Well, L1 guest is L2 I think, but I can change to explicitly mention L2 instead. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs 2020-01-29 14:45 [Xen-devel] [PATCH v2 0/2] nvmx: implement support for MSR bitmaps Roger Pau Monne 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 1/2] " Roger Pau Monne @ 2020-01-29 14:45 ` Roger Pau Monne 2020-02-03 8:07 ` Tian, Kevin 1 sibling, 1 reply; 6+ messages in thread From: Roger Pau Monne @ 2020-01-29 14:45 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Jun Nakajima, Wei Liu, Andrew Cooper, Roger Pau Monne Nested VMX doesn't expose support for SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE, SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or SECONDARY_EXEC_APIC_REGISTER_VIRT, and hence the x2APIC MSRs should always be trapped in the nested guest MSR bitmap, or else a nested guest could access the hardware x2APIC MSRs given certain conditions. Accessing the hardware MSRs could be achieved by forcing the L0 Xen to use SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or SECONDARY_EXEC_APIC_REGISTER_VIRT (if supported), and then creating a L2 guest with a MSR bitmap that doesn't trap accesses to the x2APIC MSR range. Then OR'ing both L0 and L1 MSR bitmaps would result in a bitmap that doesn't trap certain x2APIC MSRs and a VMCS that doesn't have SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or SECONDARY_EXEC_APIC_REGISTER_VIRT set either. Fix this by making sure x2APIC MSRs are always trapped in the nested MSR bitmap. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version (split from #1 patch). - Use non-locked set_bit. --- xen/arch/x86/hvm/vmx/vvmx.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index c35b4bab84..69dd4cf6ea 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -589,6 +589,16 @@ static void update_msrbitmap(struct vcpu *v) v->arch.hvm.vmx.msr_bitmap->write_high, sizeof(msr_bitmap->write_high) * 8); + /* + * Nested VMX doesn't support any x2APIC hardware virtualization, so + * make sure all the x2APIC MSRs are trapped. + */ + for ( msr = MSR_X2APIC_FIRST; msr <= MSR_X2APIC_FIRST + 0xff; msr++ ) + { + __set_bit(msr, msr_bitmap->read_low); + __set_bit(msr, msr_bitmap->write_low); + } + unmap_domain_page(msr_bitmap); __vmwrite(MSR_BITMAP, page_to_maddr(nvmx->msr_merged)); -- 2.25.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs Roger Pau Monne @ 2020-02-03 8:07 ` Tian, Kevin 0 siblings, 0 replies; 6+ messages in thread From: Tian, Kevin @ 2020-02-03 8:07 UTC (permalink / raw) To: Roger Pau Monne, xen-devel; +Cc: Andrew Cooper, Wei Liu, Nakajima, Jun > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Wednesday, January 29, 2020 10:45 PM > > Nested VMX doesn't expose support for > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE, > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or > SECONDARY_EXEC_APIC_REGISTER_VIRT, and hence the x2APIC MSRs should > always be trapped in the nested guest MSR bitmap, or else a nested > guest could access the hardware x2APIC MSRs given certain conditions. > > Accessing the hardware MSRs could be achieved by forcing the L0 Xen to > use SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or > SECONDARY_EXEC_APIC_REGISTER_VIRT (if supported), and then creating a > L2 guest with a MSR bitmap that doesn't trap accesses to the x2APIC > MSR range. Then OR'ing both L0 and L1 MSR bitmaps would result in a > bitmap that doesn't trap certain x2APIC MSRs and a VMCS that doesn't > have SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE and > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY or > SECONDARY_EXEC_APIC_REGISTER_VIRT set either. > > Fix this by making sure x2APIC MSRs are always trapped in the nested > MSR bitmap. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-03 17:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-29 14:45 [Xen-devel] [PATCH v2 0/2] nvmx: implement support for MSR bitmaps Roger Pau Monne 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 1/2] " Roger Pau Monne 2020-02-03 8:05 ` Tian, Kevin 2020-02-03 17:13 ` Roger Pau Monné 2020-01-29 14:45 ` [Xen-devel] [PATCH v2 2/2] nvmx: always trap accesses to x2APIC MSRs Roger Pau Monne 2020-02-03 8:07 ` Tian, Kevin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.