linux-kernel - Re: [PATCH 09/10] KVM: nVMX: include shadow vmcs12 in nested state

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: <21c6f364-d2e8-df22-ec7f-3d76c1a0f2be@redhat.com>
Date:   Tue, 31 Jul 2018 09:39:42 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Jim Mattson <jmattson@...gle.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 30/07/2018 21:11, Jim Mattson wrote:
> Does this work with CONFIG_HARDENED_USERCOPY?

Yes, kmalloc objects are exempt from the check (see new_kmalloc_cache).

> 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?

Either those fields won't be used by the destination due to the VMX MSR
values, or migration should have failed due to invalid VMX MSR values.

VMCS12_SIZE is consistent with handle_vmptrld and nested_release_vmcs12.
 If we use sizeof(*vmcs12) in vmx_get_nested_state, we will have to
adjust the copy from/to cached_vmcs12 and cached_shadow_vmcs12 to also
use sizeof(*vmcs12), otherwise:

1) nested_release_vmcs12 can leak arbitrary kernel data to userspace,
because the cached_vmcs12 and cached_shadow_vmcs12 are not zeroed when
allocated.

2) even if we zeroed the allocation in enter_vmx_operation, the guest
could observe a migration because it would change the padding at the end
of the VMCS changes to zeroes; this is strictly speaking not an issue,
but it's ugly.

Paolo

> 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.