| Message ID | 739671254d8747499c659f7bfb402436@advaoptical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On March 19, 2018 11:29:45 AM EDT, Tim Shearer <TShearer@advaoptical.com> wrote: >From: Tim Shearer <tshearer@advaoptical.com> > >This reverts commit 8eb73e2d410f00d383023fe41c0c25c6195b7389. >This reverts commit d59d51f088014f25c2562de59b9abff4f42a7468. > >Reintroducing the I/O port 0x80 bypass, the removal of which incurred a >significant performance penalty for NFV guests due to excessive and >time-consuming VMEXITs. .. and adds back a security issue. Perhaps a better approach would be to add an QEMU flag to enable/disable access it port 80? Similar to how spec_ctrl is done? > >Signed-off-by: Tim Shearer <tshearer@advaoptical.com> >--- > arch/x86/kvm/vmx.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > >diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >index 051dab74e4e9..a2194fe02f2a 100644 >--- a/arch/x86/kvm/vmx.c >+++ b/arch/x86/kvm/vmx.c >@@ -928,6 +928,8 @@ static DEFINE_PER_CPU(struct list_head, >blocked_vcpu_on_cpu); > static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock); > > enum { >+ VMX_IO_BITMAP_A, >+ VMX_IO_BITMAP_B, > VMX_VMREAD_BITMAP, > VMX_VMWRITE_BITMAP, > VMX_BITMAP_NR >@@ -935,6 +937,8 @@ enum { > > static unsigned long *vmx_bitmap[VMX_BITMAP_NR]; > >+#define vmx_io_bitmap_a >(vmx_bitmap[VMX_IO_BITMAP_A]) >+#define vmx_io_bitmap_b >(vmx_bitmap[VMX_IO_BITMAP_B]) >#define vmx_vmread_bitmap >(vmx_bitmap[VMX_VMREAD_BITMAP]) >#define vmx_vmwrite_bitmap >(vmx_bitmap[VMX_VMWRITE_BITMAP]) > >@@ -3701,7 +3705,7 @@ static __init int setup_vmcs_config(struct >vmcs_config *vmcs_conf) > #endif > CPU_BASED_CR3_LOAD_EXITING | > CPU_BASED_CR3_STORE_EXITING | >- CPU_BASED_UNCOND_IO_EXITING | >+ CPU_BASED_USE_IO_BITMAPS | > CPU_BASED_MOV_DR_EXITING | > CPU_BASED_USE_TSC_OFFSETING | > CPU_BASED_INVLPG_EXITING | >@@ -5657,6 +5661,10 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > #endif > int i; > >+ /* I/O */ >+ vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a)); >+ vmcs_write64(IO_BITMAP_B, __pa(vmx_io_bitmap_b)); >+ > if (enable_shadow_vmcs) { > vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap)); > vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap)); >@@ -6985,6 +6993,15 @@ static __init int hardware_setup(void) > memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE); > memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE); > >+ /* >+ * Allow direct access to the PC debug port (it is often used for I/O >+ * delays, but the vmexits simply slow things down). >+ */ >+ memset(vmx_io_bitmap_a, 0xff, PAGE_SIZE); >+ clear_bit(0x80, vmx_io_bitmap_a); >+ >+ memset(vmx_io_bitmap_b, 0xff, PAGE_SIZE); >+ > if (setup_vmcs_config(&vmcs_config) < 0) { > r = -EIO; > goto out; >@@ -10830,8 +10847,8 @@ static int prepare_vmcs02(struct kvm_vcpu >*vcpu, struct vmcs12 *vmcs12, > } > > /* >- * A vmexit (to either L1 hypervisor or L0 userspace) is always >needed >- * for I/O port accesses. >+ * Merging of IO bitmap not currently supported. >+ * Rather, exit every time. > */ > exec_control &= ~CPU_BASED_USE_IO_BITMAPS; > exec_control |= CPU_BASED_UNCOND_IO_EXITING;
> Perhaps a better approach would be to add an QEMU flag to enable/disable > access it port 80? > > Similar to how spec_ctrl is done? Sounds good to me - I will look into it, thanks.
On 19/03/2018 18:20, Tim Shearer wrote: > Not sure I understand. CONFIG_KVM_GUEST is enabled: > CONFIG_KVM_GUEST=y > > Yep, it's an old kernel. It's used as a testbed for throughput > performance, that's how we noticed it was broken with the 4.14 host. > However, I then discovered that a third-party router VM based on the > 4.9 (IIRC) kernel behaved in a similar manner. But port 0x80 should not be used if CONFIG_KVM_GUEST=y. What driver is running in the guest, is it something that is distributed with CentOS? Also, I see that your trace has RIP values 0x483213 and 0x483215, can you identify the corresponding source code? Paolo
On March 19, 2018 3:31:24 PM PDT, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >On March 19, 2018 11:29:45 AM EDT, Tim Shearer ><TShearer@advaoptical.com> wrote: >>From: Tim Shearer <tshearer@advaoptical.com> >> >>This reverts commit 8eb73e2d410f00d383023fe41c0c25c6195b7389. >>This reverts commit d59d51f088014f25c2562de59b9abff4f42a7468. >> >>Reintroducing the I/O port 0x80 bypass, the removal of which incurred >a >>significant performance penalty for NFV guests due to excessive and >>time-consuming VMEXITs. > > >.. and adds back a security issue. > >Perhaps a better approach would be to add an QEMU flag to >enable/disable access it port 80? > >Similar to how spec_ctrl is done? >> >>Signed-off-by: Tim Shearer <tshearer@advaoptical.com> >>--- >> arch/x86/kvm/vmx.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >>diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>index 051dab74e4e9..a2194fe02f2a 100644 >>--- a/arch/x86/kvm/vmx.c >>+++ b/arch/x86/kvm/vmx.c >>@@ -928,6 +928,8 @@ static DEFINE_PER_CPU(struct list_head, >>blocked_vcpu_on_cpu); >> static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock); >> >> enum { >>+ VMX_IO_BITMAP_A, >>+ VMX_IO_BITMAP_B, >> VMX_VMREAD_BITMAP, >> VMX_VMWRITE_BITMAP, >> VMX_BITMAP_NR >>@@ -935,6 +937,8 @@ enum { >> >> static unsigned long *vmx_bitmap[VMX_BITMAP_NR]; >> >>+#define vmx_io_bitmap_a >>(vmx_bitmap[VMX_IO_BITMAP_A]) >>+#define vmx_io_bitmap_b >>(vmx_bitmap[VMX_IO_BITMAP_B]) >>#define vmx_vmread_bitmap >>(vmx_bitmap[VMX_VMREAD_BITMAP]) >>#define vmx_vmwrite_bitmap >>(vmx_bitmap[VMX_VMWRITE_BITMAP]) >> >>@@ -3701,7 +3705,7 @@ static __init int setup_vmcs_config(struct >>vmcs_config *vmcs_conf) >> #endif >> CPU_BASED_CR3_LOAD_EXITING | >> CPU_BASED_CR3_STORE_EXITING | >>- CPU_BASED_UNCOND_IO_EXITING | >>+ CPU_BASED_USE_IO_BITMAPS | >> CPU_BASED_MOV_DR_EXITING | >> CPU_BASED_USE_TSC_OFFSETING | >> CPU_BASED_INVLPG_EXITING | >>@@ -5657,6 +5661,10 @@ static void vmx_vcpu_setup(struct vcpu_vmx >*vmx) >> #endif >> int i; >> >>+ /* I/O */ >>+ vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a)); >>+ vmcs_write64(IO_BITMAP_B, __pa(vmx_io_bitmap_b)); >>+ >> if (enable_shadow_vmcs) { >> vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap)); >> vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap)); >>@@ -6985,6 +6993,15 @@ static __init int hardware_setup(void) >> memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE); >> memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE); >> >>+ /* >>+ * Allow direct access to the PC debug port (it is often used for >I/O >>+ * delays, but the vmexits simply slow things down). >>+ */ >>+ memset(vmx_io_bitmap_a, 0xff, PAGE_SIZE); >>+ clear_bit(0x80, vmx_io_bitmap_a); >>+ >>+ memset(vmx_io_bitmap_b, 0xff, PAGE_SIZE); >>+ >> if (setup_vmcs_config(&vmcs_config) < 0) { >> r = -EIO; >> goto out; >>@@ -10830,8 +10847,8 @@ static int prepare_vmcs02(struct kvm_vcpu >>*vcpu, struct vmcs12 *vmcs12, >> } >> >> /* >>- * A vmexit (to either L1 hypervisor or L0 userspace) is always >>needed >>- * for I/O port accesses. >>+ * Merging of IO bitmap not currently supported. >>+ * Rather, exit every time. >> */ >> exec_control &= ~CPU_BASED_USE_IO_BITMAPS; >> exec_control |= CPU_BASED_UNCOND_IO_EXITING; What is the security issue? Port 0x80 used for other purposes on real hardware? In that case, the host kernel would need to know about it, and could disable this hack, no? (Such a machine would have a hard time running Linux, too. That being said, I don't think it would be a bad idea to induce something like X86_FEATURE_NOIODELAY which would patch out those writes; KVM guests could set it.)
On 20/03/2018 21:43, hpa@zytor.com wrote: > What is the security issue? Port 0x80 used for other purposes on > real hardware? In that case, the host kernel would need to know > about it, and could disable this hack, no? Yes, there are DMI-based quirks. > (Such a machine would have a hard time running Linux, too. That > being said, I don't think it would be a bad idea to induce something > like X86_FEATURE_NOIODELAY which would patch out those writes; KVM > guests could set it.) We already do that in KVM guests through pvops. This flag could still be useful if the DMI-based quirks were to set it, but honestly I think that Tim has either a bad driver or some kind of misconfiguration. Paolo
> But port 0x80 should not be used if CONFIG_KVM_GUEST=y. What driver is > running in the guest, is it something that is distributed with CentOS? > > Also, I see that your trace has RIP values 0x483213 and 0x483215, can > you identify the corresponding source code? > Hi Paolo, I got a useful backtrace from the guest, which is running a packet forwarding app based on the DPDK (dpdk.org). The packet receive routine writes to 0xc070 using glibc's "outw_p" function which does an additional write to I/O port 80. Take a look at the assembly (from /usr/include/sys/io.h): outw_p (unsigned short int __value, unsigned short int __port) { __asm__ __volatile__ ("outw %w0,%w1\noutb %%al,$0x80": :"a" (__value), "Nd" (__port)); } (It's irrelevant, but 0xc070 is the Virtio Ethernet device's base address (memory mapped via UIO), plus the "VIRTIO_PCI_QUEUE_NOTIFY" offset.) It does this write for every packet that's received, causing a flood of KVM userspace context switches. It appears that this implementation was completely rewritten and the ioport stuff removed by the 16.11 DPDK release, but, given the fact the project is six years old, there are likely to be many DPDK-based VNFs floating around that could potentially be impacted. How about I supersede the patch with one that disables the VMX IO port 0x80 passthrough by default (i.e. leaving the security fix in place), but allowing it to be enabled via a "port80_passthrough" module parameter? Thanks, Tim
On 23/03/18 23:38, Tim Shearer wrote: > How about I supersede the patch with one that disables the VMX IO port > 0x80 passthrough by default (i.e. leaving the security fix in place), > but allowing it to be enabled via a "port80_passthrough" module > parameter? Thanks, Tim I think it makes more sense to make it a per-VM flag passed from QEMU command-line. As suggested by Konrad. -Liran
On Fri, Mar 23, 2018 at 08:38:57PM +0000, Tim Shearer wrote: > > But port 0x80 should not be used if CONFIG_KVM_GUEST=y. What driver is > > running in the guest, is it something that is distributed with CentOS? > > > > Also, I see that your trace has RIP values 0x483213 and 0x483215, can > > you identify the corresponding source code? > > > > Hi Paolo, > > I got a useful backtrace from the guest, which is running a packet forwarding app based on the DPDK (dpdk.org). The packet receive routine writes to 0xc070 using glibc's "outw_p" function which does an additional write to I/O port 80. Take a look at the assembly (from /usr/include/sys/io.h): > > outw_p (unsigned short int __value, unsigned short int __port) > { > __asm__ __volatile__ ("outw %w0,%w1\noutb %%al,$0x80": :"a" (__value), > "Nd" (__port)); > } > > (It's irrelevant, but 0xc070 is the Virtio Ethernet device's base address (memory mapped via UIO), plus the "VIRTIO_PCI_QUEUE_NOTIFY" offset.) > > It does this write for every packet that's received, causing a flood of KVM userspace context switches. It appears that this implementation was completely rewritten and the ioport stuff removed by the 16.11 DPDK release, but, given the fact the project is six years old, there are likely to be many DPDK-based VNFs floating around that could potentially be impacted. > > How about I supersede the patch with one that disables the VMX IO port 0x80 passthrough by default (i.e. leaving the security fix in place), but allowing it to be enabled via a "port80_passthrough" module parameter? That is certainly one way. But I would suggest you change it to allow a list of ports. So you could do 80, 23, etc.. That way if there are other issues in the future we already have the ways to fix this. Also another fix is to just binarily patch that DPDK code. That is use objdump to find this 'outb %al,0x80' and patch it over with 'nop'. > Thanks, > Tim
On 03/19/18 09:53, Konrad Rzeszutek Wilk wrote: >> >> None specifically, but it is otherwise "normal" behavior of some VMs. Apparently it used to be a common method to synchronize writes to other I/O ports. In the commit thread for the original commit no-one was able to reproduce it. There are no details on what processors are impacted or exactly what the "exceptions and instability in the host kernel" were. > > <blinks> There are OSes out there that use a "debug" port to synchronize > I/O port access? That seems ill-advised? What are those OSes? > Linux, most of its bootloaders, and a number of OSes developed after Linux. -hpa
On 03/20/18 14:29, Paolo Bonzini wrote: > On 20/03/2018 21:43, hpa@zytor.com wrote: >> What is the security issue? Port 0x80 used for other purposes on >> real hardware? In that case, the host kernel would need to know >> about it, and could disable this hack, no? > > Yes, there are DMI-based quirks. > >> (Such a machine would have a hard time running Linux, too. That >> being said, I don't think it would be a bad idea to induce something >> like X86_FEATURE_NOIODELAY which would patch out those writes; KVM >> guests could set it.) > > We already do that in KVM guests through pvops. This flag could still > be useful if the DMI-based quirks were to set it, but honestly I think > that Tim has either a bad driver or some kind of misconfiguration. I guess the security issue is that if it is permitted to *read* from port 0x80 then you can read the last value written, at least on some systems (it aliases an unused DMA page register which are RW storage at least on some systems.) This would allow the guest to snoop on activity in the host or other guests depending on what is going on. So yes... not really possible without separate read and write bitmaps :( -hpa
----- Original Message ----- > From: "H. Peter Anvin" <hpa@zytor.com> > To: "Paolo Bonzini" <pbonzini@redhat.com>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>, "Tim Shearer" > <TShearer@advaoptical.com>, kvm@vger.kernel.org > Cc: "Radim Krčmář" <rkrcmar@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Ingo Molnar" <mingo@redhat.com>, > "Andrew Honig" <ahonig@google.com>, "Quan Xu" <quan.xu0@gmail.com>, x86@kernel.org > Sent: Friday, March 23, 2018 11:15:05 PM > Subject: Re: [PATCH] KVM: VMX: Reintroduce I/O port 0x80 bypass > > On 03/20/18 14:29, Paolo Bonzini wrote: > > On 20/03/2018 21:43, hpa@zytor.com wrote: > >> What is the security issue? Port 0x80 used for other purposes on > >> real hardware? In that case, the host kernel would need to know > >> about it, and could disable this hack, no? > > > > Yes, there are DMI-based quirks. > > > >> (Such a machine would have a hard time running Linux, too. That > >> being said, I don't think it would be a bad idea to induce something > >> like X86_FEATURE_NOIODELAY which would patch out those writes; KVM > >> guests could set it.) > > > > We already do that in KVM guests through pvops. This flag could still > > be useful if the DMI-based quirks were to set it, but honestly I think > > that Tim has either a bad driver or some kind of misconfiguration. > > I guess the security issue is that if it is permitted to *read* from > port 0x80 then you can read the last value written, at least on some > systems (it aliases an unused DMA page register which are RW storage at > least on some systems.) This would allow the guest to snoop on activity > in the host or other guests depending on what is going on. No, IIRC it was just crashing the host occasionally. Paolo
On 03/23/18 15:34, Paolo Bonzini wrote: >> >> I guess the security issue is that if it is permitted to *read* from >> port 0x80 then you can read the last value written, at least on some >> systems (it aliases an unused DMA page register which are RW storage at >> least on some systems.) This would allow the guest to snoop on activity >> in the host or other guests depending on what is going on. > > No, IIRC it was just crashing the host occasionally. > In that case it should be possible to allow it IF AND ONLY IF the host uses it itself for I/O delay (not blocked by DMI). However, I suspect the above issue would exist anyway :( -hpa
We(Google) have tests where the guest floods port 0x80 by writing to it repeatedly every instruction one after another. On some platforms this would eventually cause system instability leading to a crash of the host. It doesn't happen on all the intel platforms we've tested, but it does happen on some of them. On Fri, Mar 23, 2018 at 3:44 PM H. Peter Anvin <hpa@zytor.com> wrote: > On 03/23/18 15:34, Paolo Bonzini wrote: > >> > >> I guess the security issue is that if it is permitted to *read* from > >> port 0x80 then you can read the last value written, at least on some > >> systems (it aliases an unused DMA page register which are RW storage at > >> least on some systems.) This would allow the guest to snoop on activity > >> in the host or other guests depending on what is going on. > > > > No, IIRC it was just crashing the host occasionally. > > > In that case it should be possible to allow it IF AND ONLY IF the host > uses it itself for I/O delay (not blocked by DMI). However, I suspect > the above issue would exist anyway :( > -hpa
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 051dab74e4e9..a2194fe02f2a 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -928,6 +928,8 @@ static DEFINE_PER_CPU(struct list_head, blocked_vcpu_on_cpu); static DEFINE_PER_CPU(spinlock_t, blocked_vcpu_on_cpu_lock); enum { + VMX_IO_BITMAP_A, + VMX_IO_BITMAP_B, VMX_VMREAD_BITMAP, VMX_VMWRITE_BITMAP, VMX_BITMAP_NR @@ -935,6 +937,8 @@ enum { static unsigned long *vmx_bitmap[VMX_BITMAP_NR]; +#define vmx_io_bitmap_a (vmx_bitmap[VMX_IO_BITMAP_A]) +#define vmx_io_bitmap_b (vmx_bitmap[VMX_IO_BITMAP_B]) #define vmx_vmread_bitmap (vmx_bitmap[VMX_VMREAD_BITMAP]) #define vmx_vmwrite_bitmap (vmx_bitmap[VMX_VMWRITE_BITMAP]) @@ -3701,7 +3705,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) #endif CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING | - CPU_BASED_UNCOND_IO_EXITING | + CPU_BASED_USE_IO_BITMAPS | CPU_BASED_MOV_DR_EXITING | CPU_BASED_USE_TSC_OFFSETING | CPU_BASED_INVLPG_EXITING | @@ -5657,6 +5661,10 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) #endif int i; + /* I/O */ + vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a)); + vmcs_write64(IO_BITMAP_B, __pa(vmx_io_bitmap_b)); + if (enable_shadow_vmcs) { vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap)); vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap)); @@ -6985,6 +6993,15 @@ static __init int hardware_setup(void) memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE); memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE); + /* + * Allow direct access to the PC debug port (it is often used for I/O + * delays, but the vmexits simply slow things down). + */ + memset(vmx_io_bitmap_a, 0xff, PAGE_SIZE); + clear_bit(0x80, vmx_io_bitmap_a); + + memset(vmx_io_bitmap_b, 0xff, PAGE_SIZE); + if (setup_vmcs_config(&vmcs_config) < 0) { r = -EIO; goto out; @@ -10830,8 +10847,8 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, } /* - * A vmexit (to either L1 hypervisor or L0 userspace) is always needed - * for I/O port accesses. + * Merging of IO bitmap not currently supported. + * Rather, exit every time. */ exec_control &= ~CPU_BASED_USE_IO_BITMAPS; exec_control |= CPU_BASED_UNCOND_IO_EXITING;