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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
