linux-kernel - Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4

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: <CABgObfbwAe3ut18bS2u05pAgDoUvix_N9LKMb1iBcx8xNd9dMQ@mail.gmail.com>
Date:   Tue, 14 Mar 2023 12:40:38 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Reima ISHII <ishiir@...cc.u-tokyo.ac.jp>,
        stable@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4

On Sat, Mar 11, 2023 at 1:17 AM Sean Christopherson <seanjc@...gle.com> wrote:
> > @@ -3047,6 +3047,19 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> >                                          vmcs12->guest_ia32_perf_global_ctrl)))
> >               return -EINVAL;
> >
> > +     if (CC((vmcs12->guest_cr0 & (X86_CR0_PG | X86_CR0_PE)) == X86_CR0_PG))
> > +             return -EINVAL;
> > +
> > +#ifdef CONFIG_X86_64
>
> The #ifdef isn't necessary, attempting to set for a 32-bit host should be rejected
> by nested_vmx_check_controls() since nested_vmx_setup_ctls_msrs() clears the bit.
> Ditto for the host logic related to VM_EXIT_HOST_ADDR_SPACE_SIZE, which looks
> suspiciously similar ;-)

Yeah, I noticed that too and decided that the idea could have been to
allow some dead code elimination on 32-bit kernels, so I copied what
the host state checks were doing. But if you prefer the more compact
way I am absolutely not going to complain.

> > +     if (CC(ia32e &&
> > +            (!(vmcs12->guest_cr4 & X86_CR4_PAE) ||
> > +             !(vmcs12->guest_cr0 & X86_CR0_PG))))
> > +             return -EINVAL;
>
> This is a lot easier to read IMO, and has the advantage of more precisely
> identifying the failure in the tracepoint.
>
>         if (CC(ia32e && !(vmcs12->guest_cr4 & X86_CR4_PAE)) ||
>             CC(ia32e && !(vmcs12->guest_cr4 & X86_CR0_PG)))
>                 return -EINVAL;

Looks good.  I squashed everything in.

Paolo