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

[Paolo Bonzini]

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.