linux-kernel - Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

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]

Date:   Fri, 1 Nov 2019 19:39:55 +0000
From:   "Moger, Babu" <Babu.Moger@....com>
To:     Jim Mattson <jmattson@...gle.com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "rkrcmar@...hat.com" <rkrcmar@...hat.com>,
        "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>,
        "wanpengli@...cent.com" <wanpengli@...cent.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "joro@...tes.org" <joro@...tes.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "zohar@...ux.ibm.com" <zohar@...ux.ibm.com>,
        "yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
        "nayna@...ux.ibm.com" <nayna@...ux.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally



On 11/1/19 1:35 PM, Jim Mattson wrote:
> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@....com> wrote:
>>
>> The UMIP (User-Mode Instruction Prevention) feature bit should be
>> set if the emulation (kvm_x86_ops->umip_emulated) is supported
>> which is done already.
>>
>> Remove the unconditional setting of this bit.
>>
>> Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP")
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>>  arch/x86/kvm/cpuid.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index f68c0c753c38..5b81ba5ad428 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
>>         /* cpuid 7.0.ecx*/
>>         const u32 kvm_cpuid_7_0_ecx_x86_features =
>>                 F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) |
>> -               F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
>> +               F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) |
>>                 F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
>>                 F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
>>
> 
> This isn't unconditional. This is masked by the features on the boot
> CPU. Since UMIP can be virtualized (without emulation) on CPUs that
> support UMIP, you should leave this alone.
> 

There is some inconstancy here.

static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32

unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
...

case 7: {
             ..
            entry->ecx |= f_umip;
            ..
        }

and
static bool svm_umip_emulated(void)
{
        return false;
}

static inline bool vmx_umip_emulated(void)
{
        return vmcs_config.cpu_based_2nd_exec_ctrl &
                SECONDARY_EXEC_DESC;
}


It appears that intention was to enable the UMIP if SVM or VMX support
umip emulation. But that is not how it is working now. Now it is enabled
if boot CPU supports UMIP.