On Mon, Jan 14, 2019 at 06:43:04PM -0800, Sean Christopherson wrote: > On Mon, Jan 14, 2019 at 03:47:28PM -0800, Tom Roeder wrote: > > This changes the allocation of cached_vmcs12 to use kzalloc instead of > > kmalloc. This removes the information leak found by Syzkaller (see > > Reported-by) in this case and prevents similar leaks from happening > > based on cached_vmcs12. > > Is the leak specific to vmx_set_nested_state(), e.g. can we zero out > the memory if copy_from_user() fails instead of taking the hit on every > allocation? I don't know if the leak is specific to vmx_set_nested_state. This question (and the rest of the thread from November) goes to the heart of what I wanted to get feedback about; hence the "RFC" part of the subject line. I'm new to the kernel, and I don't know all the idioms and expectations, so the follow analysis is an outsider's view of the issue at hand. What I see in this case is a field that is intended to be copied to the guest and is allocated initially with data from the kernel. I'm sure we could figure out all the current paths and error cases that we need to handle to make sure that this data never leaks. Future reviewers then also need to make sure that changes to the nested VMX code don't leak data from this field. Why not instead make sure that there isn't any data to leak in the first place? I understand that there's a cost to kzalloc vs. kmalloc, but I don't know what it is in practice; slab.c shows that the extra code for the __GFP_ZERO flag is a memset of 0 over the allocated memory. The allocation looks very infrequent for the two lines in this patch, since they occur in enter_vmx_operation. That sounds to me like the allocation only happens when the guest enables nested virtualization. Given the frequency of allocation and the relative security benefit of not having to worry about leaking the data, I'd advocate for changing it here.
