KVM: vmx: use local variable for CVP when emulating VMPTRST
diff mbox

Message ID 20180719173100.2244-1-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson July 19, 2018, 5:31 p.m. UTC
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(-)

Comments

Peter Shier July 20, 2018, 12:47 a.m. UTC | #1
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!
Paolo Bonzini July 20, 2018, 10:54 a.m. UTC | #2
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
Jim Mattson July 20, 2018, 5:01 p.m. UTC | #3
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
>
Sean Christopherson July 20, 2018, 6:09 p.m. UTC | #4
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
> >

Patch
diff mbox

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;
 	}