linux-kernel - Re: [PATCH 1/2] KVM: VMX: Sanitize VM-Entry/VM-Exit control pairs at kvm

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: <Yo/yhiVl++FTSa3S@google.com>
Date:   Thu, 26 May 2022 21:35:02 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Chenyi Qiang <chenyi.qiang@...el.com>,
        Lei Wang <lei4.wang@...el.com>
Subject: Re: [PATCH 1/2] KVM: VMX: Sanitize VM-Entry/VM-Exit control pairs at
 kvm_intel load time

On Thu, May 26, 2022, Paolo Bonzini wrote:
> On 5/25/22 23:04, Sean Christopherson wrote:
> > +#define VMCS_ENTRY_EXIT_PAIR(name, entry_action, exit_action) \
> > +	{ VM_ENTRY_##entry_action##_##name, VM_EXIT_##exit_action##_##name }
> > +
> >   static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> >   				    struct vmx_capability *vmx_cap)
> >   {
> > @@ -2473,6 +2476,24 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> >   	u64 _cpu_based_3rd_exec_control = 0;
> >   	u32 _vmexit_control = 0;
> >   	u32 _vmentry_control = 0;
> > +	int i;
> > +
> > +	/*
> > +	 * LOAD/SAVE_DEBUG_CONTROLS are absent because both are mandatory.
> > +	 * SAVE_IA32_PAT and SAVE_IA32_EFER are absent because KVM always
> > +	 * intercepts writes to PAT and EFER, i.e. never enables those controls.
> > +	 */
> > +	struct {
> > +		u32 entry_control;
> > +		u32 exit_control;
> > +	} vmcs_entry_exit_pairs[] = {
> > +		VMCS_ENTRY_EXIT_PAIR(IA32_PERF_GLOBAL_CTRL, LOAD, LOAD),
> > +		VMCS_ENTRY_EXIT_PAIR(IA32_PAT, LOAD, LOAD),
> > +		VMCS_ENTRY_EXIT_PAIR(IA32_EFER, LOAD, LOAD),
> > +		VMCS_ENTRY_EXIT_PAIR(BNDCFGS, LOAD, CLEAR),
> > +		VMCS_ENTRY_EXIT_PAIR(IA32_RTIT_CTL, LOAD, CLEAR),
> > +		VMCS_ENTRY_EXIT_PAIR(IA32_LBR_CTL, LOAD, CLEAR),
> 
> No macros please, it's just as clear to expand them especially since the
> #define is far from the struct definition.

It's not for clarity, it's to prevent plopping an EXIT control into the ENTRY
slot and vice versa.  I have a hell of a time trying to visually differentiate
those, and a buggy pair isn't guaranteed to be detected at runtime, e.g. if both
are swapped, all bets are off, and if one is duplicated, odds the warn may or may
not show up unless hardware actually supports at least one of the controls, if not
both.

With this, swapping LOAD and LOAD is obviously a nop, and swapping LOAD and CLEAR
will generate a compiler error.

FWIW, I did originally have the array declared as static __initdata immediately
after the #define.  I moved away from that because __initdata doesn't play nice
with const, but then of course I forgot to add back the "const".  /facepalm