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

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

[Vitaly Kuznetsov]

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@intel.com>
Fixes: 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer migration")
Fixes: 83d31e5271ac ("KVM: nVMX: fixes for preemption timer migration")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- The patch breaks ABI so it needs to go into 5.8.

- This is a successor of "[PATCH] KVM: nVMX: properly pad struct
 kvm_vmx_nested_state_hdr"
---
 Documentation/virt/kvm/api.rst  | 5 +++--
 arch/x86/include/uapi/asm/kvm.h | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 320788f81a05..e75992ad856a 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -4345,8 +4345,9 @@ Errors:
 	struct {
 		__u16 flags;
 	} smm;
-
-	__u32 flags;
+	__u16 pad16;
+	__u32 pad32;
+	__u64 flags;
 	__u64 preemption_timer_deadline;
   };
 
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 0780f97c1850..4ecc6bd49818 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -414,8 +414,9 @@ struct kvm_vmx_nested_state_hdr {
 	struct {
 		__u16 flags;
 	} smm;
-
-	__u32 flags;
+	__u16 pad16;
+	__u32 pad32;
+	__u64 flags;
 	__u64 preemption_timer_deadline;
 };
 
-- 
2.25.4

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

[Jim Mattson]

On Mon, Jul 13, 2020 at 9:22 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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@intel.com>
> Fixes: 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer migration")
> Fixes: 83d31e5271ac ("KVM: nVMX: fixes for preemption timer migration")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Oops!

Reviewed-by: Jim Mattson <jmattson@google.com>

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

[Jim Mattson]

On Mon, Jul 13, 2020 at 11:23 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Mon, Jul 13, 2020 at 9:22 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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@intel.com>
> > Fixes: 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer migration")
> > Fixes: 83d31e5271ac ("KVM: nVMX: fixes for preemption timer migration")
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Oops!
>
> Reviewed-by: Jim Mattson <jmattson@google.com>

Whatever happened to this?

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@google.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 9:22 AM Vitaly Kuznetsov <vkuznets@redhat.com> 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@intel.com>
> > > Fixes: 850448f35aaf ("KVM: nVMX: Fix VMX preemption timer migration")
> > > Fixes: 83d31e5271ac ("KVM: nVMX: fixes for preemption timer migration")
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >
> > Oops!
> >
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> 
> 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.

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

[Paolo Bonzini]

On 27/08/20 22:40, Sean Christopherson wrote:
> 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.

In general userspace (as a hygiene/future-proofing measure) should
generally zero the contents of structs before filling in some fields
only.  There was no guarantee that smm wouldn't grow new fields that
would have occupied the padding, for example.  The general solution we
use is flags fields and checking them.

(The original KVM_GET/SET_NESTED_STATE patches did add a generic flags
fields, but not a VMX-specific one field).

Paolo

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

[Paolo Bonzini]

On 27/08/20 22:40, Sean Christopherson wrote:
> 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.

In general userspace (as a hygiene/future-proofing measure) should
generally zero the contents of structs before filling in some fields
only.  There was no guarantee that smm wouldn't grow new fields that
would have occupied the padding, for example.  The general solution we
use is flags fields and checking them.

(The original KVM_GET/SET_NESTED_STATE patches did add a generic flags
fields, but not a VMX-specific one).

Paolo