* [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
@ 2020-07-13 16:22 Vitaly Kuznetsov
2020-07-13 18:23 ` Jim Mattson
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-07-13 16:22 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Peter Shier, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
2020-07-13 16:22 [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr Vitaly Kuznetsov
@ 2020-07-13 18:23 ` Jim Mattson
2020-08-27 18:25 ` Jim Mattson
0 siblings, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2020-07-13 18:23 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm list, Paolo Bonzini, Sean Christopherson, Wanpeng Li,
Peter Shier, LKML
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
2020-07-13 18:23 ` Jim Mattson
@ 2020-08-27 18:25 ` Jim Mattson
2020-08-27 20:40 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2020-08-27 18:25 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm list, Paolo Bonzini, Sean Christopherson, Wanpeng Li,
Peter Shier, LKML
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?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
2020-08-27 18:25 ` Jim Mattson
@ 2020-08-27 20:40 ` Sean Christopherson
2020-08-30 9:52 ` Paolo Bonzini
2020-08-30 9:52 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-08-27 20:40 UTC (permalink / raw)
To: Jim Mattson
Cc: Vitaly Kuznetsov, kvm list, Paolo Bonzini, Wanpeng Li, Peter Shier, LKML
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
2020-08-27 20:40 ` Sean Christopherson
@ 2020-08-30 9:52 ` Paolo Bonzini
2020-08-30 9:52 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-08-30 9:52 UTC (permalink / raw)
To: Sean Christopherson, Jim Mattson
Cc: Vitaly Kuznetsov, kvm list, Wanpeng Li, Peter Shier, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr
2020-08-27 20:40 ` Sean Christopherson
2020-08-30 9:52 ` Paolo Bonzini
@ 2020-08-30 9:52 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-08-30 9:52 UTC (permalink / raw)
To: Sean Christopherson, Jim Mattson
Cc: Vitaly Kuznetsov, kvm list, Wanpeng Li, Peter Shier, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-30 9:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 16:22 [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state_hdr Vitaly Kuznetsov
2020-07-13 18:23 ` Jim Mattson
2020-08-27 18:25 ` Jim Mattson
2020-08-27 20:40 ` Sean Christopherson
2020-08-30 9:52 ` Paolo Bonzini
2020-08-30 9:52 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.