linux-kernel - Re: [PATCH v2] KVM: nVMX: fix the layout of struct kvm_vmx_nested_state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200827204020.GE22351@sjchrist-ice>
Date:   Thu, 27 Aug 2020 13:40:20 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jim Mattson <jmattson@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        kvm list <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Peter Shier <pshier@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] KVM: nVMX: fix the layout of struct
 kvm_vmx_nested_state_hdr

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@...gle.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 9:22 AM Vitaly Kuznetsov <vkuznets@...hat.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@...el.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@...hat.com>
> >
> > Oops!
> >
> > Reviewed-by: Jim Mattson <jmattson@...gle.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.