On 23/01/19 19:25, Tom Roeder wrote: > On Tue, Jan 15, 2019 at 11:15:51AM +0100, Paolo Bonzini wrote: >> On 15/01/19 03:43, Sean Christopherson wrote: >>>> - vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL); >>>> + vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL); >>>> if (!vmx->nested.cached_vmcs12) >>>> goto out_cached_vmcs12; >>> Obviously not your code, but why do we allocate VMCS12_SIZE instead of >>> sizeof(struct vmcs12)? I get why we require userspace to reserve the >>> full 4k, but I don't understand why KVM needs to allocate the reserved >>> bytes internally. >> >> It's just cleaner and shorter code to copy everything in and out, >> instead of having to explicitly zero the slack. > > Could you please clarify? I don't see code that copies everything in and > out, but it depends on what you mean by "everything". In the context of > this email exchange, I assumed that "everything" was "all 4k > (VMCS12_SIZE)". I was thinking of vmx_get_nested_state, but actually it only copies sizeof(*vmcs12). However, that is the place where we should copy 4k out of it, including the zeroes. Otherwise, our userspace clients (which doesn't know sizeof(*vmcs12) could leak uninitialized data of their own. Paolo > But it looks to me like the code doesn't copy 4k in and out, but rather > only ever copies sizeof(struct vmcs12) in and out. The copy_from_user > and copy_to_user cases in nested.c use sizeof(*vmcs12), which is > sizeof(struct vmcs12). > > So maybe can switch to allocating sizeof(struct vmcs12). Is this > correct, or is there some other reason to allocate the larger size? >
