Re: [PATCH] KVM: nVMX: properly pad struct kvm_vmx_nested_state_hdr

[Sean Christopherson]

On Mon, Jul 27, 2020 at 06:16:56PM +0200, Paolo Bonzini wrote:
> On 27/07/20 17:46, Sean Christopherson wrote:
> > All the above being said, after looking at the whole picture I think padding
> > the header is a moot point.  The header is padded out to 120 bytes[*] when
> > including in the full nested state, and KVM only ever consumes the header in
> > the context of the full nested state.  I.e. if there's garbage at offset 6,
> > odds are there's going to be garbage at offset 18, so internally padding the
> > header does nothing.
> 
> Yes, that was what I was hinting at with "it might as well send it now"
> (i.e., after the patch).
> 
> (All of this is moot for userspace that just uses KVM_GET_NESTED_STATE
> and passes it back to KVM_SET_NESTED_STATE).
> 
> > KVM should be checking that the unused bytes of (sizeof(pad) - sizeof(vmx/svm))
> > is zero if we want to expand into the padding in the future.  Right now we're
> > relying on userspace to zero allocate the struct without enforcing it.
> 
> The alternative, which is almost as good, is to only use these extra
> fields which could be garbage if the flags are not set, and check the
> flags (see the patches I have sent earlier today).
> 
> The chance of the flags passing the check will decrease over time as
> more flags are added; but the chance of having buggy userspace that
> sends down garbage also will.

Ah, I see what you're saying.  Ya, that makes sense.

> > [*] Amusing side note, the comment in the header is wrong.  It states "pad
> >     the header to 128 bytes", but only pads it to 120 bytes, because union.
> > 
> > /* for KVM_CAP_NESTED_STATE */
> > struct kvm_nested_state {
> > 	__u16 flags;
> > 	__u16 format;
> > 	__u32 size;
> > 
> > 	union {
> > 		struct kvm_vmx_nested_state_hdr vmx;
> > 		struct kvm_svm_nested_state_hdr svm;
> > 
> > 		/* Pad the header to 128 bytes.  */
> > 		__u8 pad[120];
> > 	} hdr;
> 
> There are 8 bytes before the union, and it's not a coincidence. :)
> "Header" refers to the stuff before the data region.

Ugh, then 'hdr' probably should be named vendor_header or something.