lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALMp9eSj+rmgvcEb8=W1M+Z0S4m5hKYjXvxTd6DhQe+SLCi=Kw@mail.gmail.com>
Date:   Mon, 30 Jul 2018 12:11:52 -0700
From:   Jim Mattson <jmattson@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        Liran Alon <liran.alon@...cle.com>,
        KarimAllah Ahmed <karahmed@...zon.de>,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [PATCH 09/10] KVM: nVMX: include shadow vmcs12 in nested state

On Sat, Jul 28, 2018 at 4:10 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
> The shadow vmcs12 cannot be flushed on KVM_GET_NESTED_STATE,
> because at that point guest memory is assumed by userspace to
> be immutable.  Capture the cache in vmx_get_nested_state, adding
> another page at the end if there is an active shadow vmcs12.
>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/vmx.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b5ee9e08bb48..ce8c0c759a19 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -13167,9 +13167,15 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>                 kvm_state.vmx.vmxon_pa = vmx->nested.vmxon_ptr;
>                 kvm_state.vmx.vmcs_pa = vmx->nested.current_vmptr;
>
> -               if (vmx->nested.current_vmptr != -1ull)
> +               if (vmx->nested.current_vmptr != -1ull) {
>                         kvm_state.size += VMCS12_SIZE;
>
> +                       if (is_guest_mode(vcpu) &&
> +                           nested_cpu_has_shadow_vmcs(vmcs12) &&
> +                           vmcs12->vmcs_link_pointer != -1ull)
> +                               kvm_state.size += VMCS12_SIZE;
> +               }
> +
>                 if (vmx->nested.smm.vmxon)
>                         kvm_state.vmx.smm.flags |= KVM_STATE_NESTED_SMM_VMXON;
>
> @@ -13208,6 +13214,13 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>         if (copy_to_user(user_kvm_nested_state->data, vmcs12, sizeof(*vmcs12)))
>                 return -EFAULT;
>
> +       if (nested_cpu_has_shadow_vmcs(vmcs12) &&
> +           vmcs12->vmcs_link_pointer != -1ull) {
> +               if (copy_to_user(user_kvm_nested_state->data + VMCS12_SIZE,
> +                                get_shadow_vmcs12(vcpu), sizeof(*vmcs12)))

Does this work with CONFIG_HARDENED_USERCOPY?

Is VMCS12_SIZE better than sizeof(*vmcs12)? What if we are migrating
to a destination where sizeof(*vmcs12) is larger than it is on the
source? If userspace doesn't zero the buffer before calling the ioctl,
then the destination may interpret nonsense as actual VMCS field
values. However, if we copy the entire page from the kernel, then we
know that anything beyond the source's sizeof(*vmcs12) will be zero.

> +                       return -EFAULT;
> +       }
> +
>  out:
>         return kvm_state.size;
>  }
> @@ -13288,6 +13301,22 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>         vmx->nested.nested_run_pending =
>                 !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
>
> +       if (nested_cpu_has_shadow_vmcs(vmcs12) &&
> +           vmcs12->vmcs_link_pointer != -1ull) {
> +               struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> +               if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> +                       return -EINVAL;
> +
> +               if (copy_from_user(shadow_vmcs12,
> +                                  user_kvm_nested_state->data + VMCS12_SIZE,
> +                                  sizeof(*vmcs12)))
> +                       return -EFAULT;
> +
> +               if (shadow_vmcs12->hdr.revision_id != VMCS12_REVISION ||
> +                   !shadow_vmcs12->hdr.shadow_vmcs)
> +                       return -EINVAL;
> +       }
> +
>         if (check_vmentry_prereqs(vcpu, vmcs12) ||
>             check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
>                 return -EINVAL;
> --
> 1.8.3.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ