| Message ID | 20180719173100.2244-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 19, 2018 at 10:31 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Do not expose the address of vmx->nested.current_vmptr to > kvm_write_guest_virt_system() as the resulting __copy_to_user() > call will trigger a WARN when CONFIG_HARDENED_USERCOPY is > enabled. > > Opportunistically clean up variable names in handle_vmptrst() > to improve readability, e.g. vmcs_gva is misleading as the > memory operand of VMPSTR is plain memory, not a VMCS. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- Tested-by: Peter Shier <pshier@google.com> Reviewed-by: Peter Shier <pshier@google.com> Thanks Sean!
On 20/07/2018 02:47, Peter Shier wrote: > Tested-by: Peter Shier <pshier@google.com> > Reviewed-by: Peter Shier <pshier@google.com> Thanks Peter. I'm applying the patch to kvm/master. Paolo
On Thu, Jul 19, 2018 at 10:31 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > Do not expose the address of vmx->nested.current_vmptr to > kvm_write_guest_virt_system() as the resulting __copy_to_user() > call will trigger a WARN when CONFIG_HARDENED_USERCOPY is > enabled. > > Opportunistically clean up variable names in handle_vmptrst() > to improve readability, e.g. vmcs_gva is misleading as the > memory operand of VMPSTR is plain memory, not a VMCS. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index e30da9a2430c..6688dcf314d3 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -8480,21 +8480,20 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu) > /* Emulate the VMPTRST instruction */ > static int handle_vmptrst(struct kvm_vcpu *vcpu) > { > - unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION); > - u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); > - gva_t vmcs_gva; > + unsigned long exit_qual = vmcs_readl(EXIT_QUALIFICATION); > + u32 instr_info = vmcs_read32(VMX_INSTRUCTION_INFO); > + gpa_t cvp = to_vmx(vcpu)->nested.current_vmptr; > struct x86_exception e; > + gva_t gva; > > if (!nested_vmx_check_permission(vcpu)) > return 1; > > - if (get_vmx_mem_address(vcpu, exit_qualification, > - vmx_instruction_info, true, &vmcs_gva)) > + if (get_vmx_mem_address(vcpu, exit_qual, instr_info, true, &gva)) > return 1; > /* *_system ok, nested_vmx_check_permission has verified cpl=0 */ > - if (kvm_write_guest_virt_system(vcpu, vmcs_gva, > - (void *)&to_vmx(vcpu)->nested.current_vmptr, > - sizeof(u64), &e)) { > + if (kvm_write_guest_virt_system(vcpu, gva, (void *)&cvp, > + sizeof(gpa_t), &e)) { I actually think sizeof(u64) was better here, since the SDM says: "64-bit in-memory destination operand <- current-VMCS pointer;" But as long as a gpa_t is always 64-bits, this is okay. > kvm_inject_page_fault(vcpu, &e); > return 1; > } > -- > 2.18.0 >
On Fri, 2018-07-20 at 10:01 -0700, Jim Mattson wrote: > On Thu, Jul 19, 2018 at 10:31 AM, Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > Do not expose the address of vmx->nested.current_vmptr to > > kvm_write_guest_virt_system() as the resulting __copy_to_user() > > call will trigger a WARN when CONFIG_HARDENED_USERCOPY is > > enabled. > > > > Opportunistically clean up variable names in handle_vmptrst() > > to improve readability, e.g. vmcs_gva is misleading as the > > memory operand of VMPSTR is plain memory, not a VMCS. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/vmx.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index e30da9a2430c..6688dcf314d3 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -8480,21 +8480,20 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu) > > /* Emulate the VMPTRST instruction */ > > static int handle_vmptrst(struct kvm_vcpu *vcpu) > > { > > - unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION); > > - u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); > > - gva_t vmcs_gva; > > + unsigned long exit_qual = vmcs_readl(EXIT_QUALIFICATION); > > + u32 instr_info = vmcs_read32(VMX_INSTRUCTION_INFO); > > + gpa_t cvp = to_vmx(vcpu)->nested.current_vmptr; > > struct x86_exception e; > > + gva_t gva; > > > > if (!nested_vmx_check_permission(vcpu)) > > return 1; > > > > - if (get_vmx_mem_address(vcpu, exit_qualification, > > - vmx_instruction_info, true, &vmcs_gva)) > > + if (get_vmx_mem_address(vcpu, exit_qual, instr_info, true, &gva)) > > return 1; > > /* *_system ok, nested_vmx_check_permission has verified cpl=0 */ > > - if (kvm_write_guest_virt_system(vcpu, vmcs_gva, > > - (void *)&to_vmx(vcpu)->nested.current_vmptr, > > - sizeof(u64), &e)) { > > + if (kvm_write_guest_virt_system(vcpu, gva, (void *)&cvp, > > + sizeof(gpa_t), &e)) { > I actually think sizeof(u64) was better here, since the SDM says: > "64-bit in-memory destination operand <- current-VMCS pointer;" But as > long as a gpa_t is always 64-bits, this is okay. My thinking was that it would be preferable to botch the emulation versus causing a buffer overrun in the host, though that thinking assumes gpa_t could be smaller than u64 and not vice versa. I agree a better fix would be to declare cvp as a u64 so we can write 64-bits regardless of gpa_t. > > > > kvm_inject_page_fault(vcpu, &e); > > return 1; > > } > > -- > > 2.18.0 > >
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index e30da9a2430c..6688dcf314d3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -8480,21 +8480,20 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu) /* Emulate the VMPTRST instruction */ static int handle_vmptrst(struct kvm_vcpu *vcpu) { - unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION); - u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); - gva_t vmcs_gva; + unsigned long exit_qual = vmcs_readl(EXIT_QUALIFICATION); + u32 instr_info = vmcs_read32(VMX_INSTRUCTION_INFO); + gpa_t cvp = to_vmx(vcpu)->nested.current_vmptr; struct x86_exception e; + gva_t gva; if (!nested_vmx_check_permission(vcpu)) return 1; - if (get_vmx_mem_address(vcpu, exit_qualification, - vmx_instruction_info, true, &vmcs_gva)) + if (get_vmx_mem_address(vcpu, exit_qual, instr_info, true, &gva)) return 1; /* *_system ok, nested_vmx_check_permission has verified cpl=0 */ - if (kvm_write_guest_virt_system(vcpu, vmcs_gva, - (void *)&to_vmx(vcpu)->nested.current_vmptr, - sizeof(u64), &e)) { + if (kvm_write_guest_virt_system(vcpu, gva, (void *)&cvp, + sizeof(gpa_t), &e)) { kvm_inject_page_fault(vcpu, &e); return 1; }
Do not expose the address of vmx->nested.current_vmptr to kvm_write_guest_virt_system() as the resulting __copy_to_user() call will trigger a WARN when CONFIG_HARDENED_USERCOPY is enabled. Opportunistically clean up variable names in handle_vmptrst() to improve readability, e.g. vmcs_gva is misleading as the memory operand of VMPSTR is plain memory, not a VMCS. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)