linux-kernel - Re: [PATCH] KVM: nVMX: Tweak handling of failure code for nested VM-Enter failure

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: <20200428161740.GE12735@linux.intel.com>
Date:   Tue, 28 Apr 2020 09:17:40 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jim Mattson <jmattson@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>, kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: nVMX: Tweak handling of failure code for nested
 VM-Enter failure

On Fri, Apr 24, 2020 at 10:47:04AM -0700, Jim Mattson wrote:
> On Fri, Apr 24, 2020 at 10:19 AM Sean Christopherson
> <sean.j.christopherson@...el.com> wrote:
> >         if (from_vmentry) {
> >                 exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> > -               exit_qual = nested_vmx_load_msr(vcpu,
> > -                                               vmcs12->vm_entry_msr_load_addr,
> > -                                               vmcs12->vm_entry_msr_load_count);
> > -               if (exit_qual)
> > +               failed_msr = nested_vmx_load_msr(vcpu,
> > +                                                vmcs12->vm_entry_msr_load_addr,
> > +                                                vmcs12->vm_entry_msr_load_count);
> > +               if (failed_msr) {
> > +                       entry_failure_code = failed_msr;
> 
> This assignment is a bit dodgy from a type perspective, and suggests
> that perhaps a better type for the local variable is an
> undiscriminated union of the enumerated type and a sufficiently large
> unsigned integer type. But I won't be a stickler if you add a comment.
> :-)

This is a bit ugly.  A union doesn't work well because writing the enum
field isn't guaranteed to write the full width of the union, i.e. could
lead to uninitialized variable usage without additional initialization
of the union.  The reverse is true as well, e.g. if the compiler sizes the
enum to be larger than an unisigned int.

Rather than use a common local variable, I think it's best to set vmcs12
directly.  That also provides an opportunity to set exit_reason on demand
instead of speculatively setting it, which has always bugged me.

v2 incoming.