linux-kernel - Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check

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] [day] [month] [year] [list]

Message-ID: <Y1r5jpRxJeDMac6T@google.com>
Date:   Thu, 27 Oct 2022 21:35:10 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check

On Thu, Oct 27, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@...gle.com> writes:
> 
> > On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
> >> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
> >> need to be adjusted to avoid the situation when the feature is exposed
> >> to the guest but there's no corresponding eVMCS field[s] for it. This
> >> is not obvious and fragile.
> >
> > Eh, either way is fragile, the only difference is what goes wrong when it breaks.
> >
> > At the risk of making this overly verbose, what about requiring developers to
> > explicitly define whether or not a new control is support?  E.g. keep the
> > EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
> > feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.
> >
> > That way the eVMCS "supported" controls don't need to include the ALWAYSON
> > controls, and anytime someone adds a new control, they'll have to stop and think
> > about eVMCS.
> 
> Is this a good thing or a bad one? :-) I'm not against being extra
> verbose but adding a new feature to EVMCS1_SUPPORTED_* (even when there
> is a corresponding field) requires testing or a
> evmcs_has_perf_global_ctrl()-like story may happen and such testing
> would require access to Windows/Hyper-V images. This sounds like an
> extra burden for contributors. IMO it's OK if new features are
> mechanically added to EVMCS1_UNSUPPORTED_* on the grounds that it
> wasn't tested but then it's not much different from "unsupported by
> default" (my approach). So I'm on the fence here.

Yeah, I was hoping the compile-time asserts would buy us full protection, i.e. I
was hoping to avoid the sanitization, but I don't see a way to handle the case
where Hyper-V starts advertising a feature that was previously unsupported :-(

I'm a-ok going with SUPPORTED only, I'm on the fence too.