Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr

[Sean Christopherson]

On Thu, Aug 27, 2020 at 11:25:25AM -0700, Jim Mattson wrote:
> On Mon, Jul 13, 2020 at 11:23 AM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 13, 2020 at 9:22 AM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
> > >
> > > Before commit 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer
> > > migration") struct kvm_vmx_nested_state_hdr looked like:
> > >
> > > struct kvm_vmx_nested_state_hdr {
> > >         __u64 vmxon_pa;
> > >         __u64 vmcs12_pa;
> > >         struct {
> > >                 __u16 flags;
> > >         } smm;
> > > }
> > >
> > > The ABI got broken by the above mentioned commit and an attempt
> > > to fix that was made in commit 83d31e5271ac ("KVM: nVMX: fixes for
> > > preemption timer migration") which made the structure look like:
> > >
> > > struct kvm_vmx_nested_state_hdr {
> > >         __u64 vmxon_pa;
> > >         __u64 vmcs12_pa;
> > >         struct {
> > >                 __u16 flags;
> > >         } smm;
> > >         __u32 flags;
> > >         __u64 preemption_timer_deadline;
> > > };
> > >
> > > The problem with this layout is that before both changes compilers were
> > > allocating 24 bytes for this and although smm.flags is padded to 8 bytes,
> > > it is initialized as a 2 byte value. Chances are that legacy userspaces
> > > using old layout will be passing uninitialized bytes which will slip into
> > > what is now known as 'flags'.
> > >
> > > Suggested-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > Fixes: 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer migration")
> > > Fixes: 83d31e5271ac ("KVM: nVMX: fixes for preemption timer migration")
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> >
> > Oops!
> >
> > Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> 
> Whatever happened to this?

Paolo pushed an alternative solution for 5.8, commit 5e105c88ab485 ("KVM:
nVMX: check for invalid hdr.vmx.flags").  His argument was that there was
no point in adding proper padding since we already broke the ABI, i.e.
damage done.

So rather than pad the struct, which doesn't magically fix the ABI for old
userspace, just check for unsupported flags.  That gives decent odds of
failing the ioctl() for old userspace if it's passing garbage (through no
fault of its own), prevents new userspace from setting unsupported flags,
and allows KVM to grow the struct by conditioning consumption of new fields
on an associated flag.