linux-kernel - Re: CONFIG_X86_HYPERVISOR (was: Re: [PATCH AUTOSEL 5.10 2/6] x86/cpu: Don't clear X86_FEATURE_LAHF_LM flag in init_amd_k8() on AMD when running in a virtual machine)

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: <20250423072017.GAaAiUsYzDOdt7cmp2@renoirsky.local>
Date: Wed, 23 Apr 2025 09:20:17 +0200
From: Borislav Petkov <bp@...en8.de>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Pavel Machek <pavel@...x.de>, Sasha Levin <sashal@...nel.org>,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	Max Grobecker <max@...becker.info>, Ingo Molnar <mingo@...nel.org>,
	tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
	x86@...nel.org, thomas.lendacky@....com, perry.yuan@....com,
	mario.limonciello@....com, riel@...riel.com, mjguzik@...il.com,
	darwi@...utronix.de, Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: CONFIG_X86_HYPERVISOR (was: Re: [PATCH AUTOSEL 5.10 2/6]
 x86/cpu: Don't clear X86_FEATURE_LAHF_LM flag in init_amd_k8() on AMD when
 running in a virtual machine)

On Tue, Apr 22, 2025 at 12:48:44PM -0700, Sean Christopherson wrote:
> I did a quick pass.

You couldn't resist, I know. Doing something else for a change is
always cool.

:-P

> Most of the usage is "fine".  E.g. explicit PV code, cases
> where checking for HYPERVISOR is the least awful option, etc.
> 
> Looks sketchy, might be worth investigating?

Oh, I will, it is on my
do-this-while-waiting-for-compile/test-to-finish. ;-P

> --------------------------------------------
>   arch/x86/kernel/cpu/amd.c:              if (!cpu_has(c, X86_FEATURE_HYPERVISOR) &&

So that first one is to set CC_ATTR_HOST_SEV_SNP when we really are
a SNP host. I'll go through the rest slowly.

>   arch/x86/kernel/cpu/amd.c:      if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_IBPB_BRTYPE)) {
>   arch/x86/kernel/cpu/amd.c:      if (c->x86_model < 0x14 && cpu_has(c, X86_FEATURE_LAHF_LM) && !cpu_has(c, X86_FEATURE_HYPERVISOR)) {
>   arch/x86/kernel/cpu/amd.c:      if (!cpu_has(c, X86_FEATURE_HYPERVISOR)) {
>   arch/x86/kernel/cpu/amd.c:      if (!cpu_has(c, X86_FEATURE_HYPERVISOR)) {
>   arch/x86/kernel/cpu/amd.c:      if (cpu_has(c, X86_FEATURE_HYPERVISOR))
>   arch/x86/kernel/cpu/amd.c:      if (!cpu_has(c, X86_FEATURE_HYPERVISOR)) {
>   arch/x86/kernel/cpu/amd.c:      if (!cpu_has(c, X86_FEATURE_HYPERVISOR))
>   arch/x86/kernel/cpu/topology_amd.c:             if (!boot_cpu_has(X86_FEATURE_HYPERVISOR) && tscan->c->x86_model <= 0x3) {
>   arch/x86/mm/init_64.c:  if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
>   arch/x86/mm/pat/set_memory.c:   return !cpu_feature_enabled(X86_FEATURE_HYPERVISOR);
>   drivers/platform/x86/intel/pmc/pltdrv.c:        if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) && !xen_initial_domain())
>   drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c: if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR))
> --------------------------------------
> 
> 
> Could do with some love, but not horrible.
> ------------------------------------------
> Eww.  Optimization to lessen the pain of DR7 interception.  It'd be nice to clean
> this up at some point, especially with things like SEV-ES with DebugSwap, where
> DR7 is never intercepted.
>   arch/x86/include/asm/debugreg.h:        if (static_cpu_has(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
>   arch/x86/kernel/hw_breakpoint.c:                 * When in guest (X86_FEATURE_HYPERVISOR), local_db_save()

Patch adding it says "Because DRn access is 'difficult' with virt;..."
so yeah. I guess we need to agree how to do debug exceptions in guests.
Probably start documenting it and then have guest and host adhere to
that. I'm talking completely without having looked at what the code does
but the "handshake" agreement should be something like this and then we
can start simplifying code...

> This usage should be restricted to just the FMS matching, but unfortunately
> needs to be kept for that check.
>   arch/x86/kernel/cpu/bus_lock.c: if (boot_cpu_has(X86_FEATURE_HYPERVISOR))

I have no idea why that was added - perhaps to avoid split-lock related
#ACs on guests...

/does more git archeology...

Aha, I see it: 6650cdd9a8ccf

Although this doesn't explicitly comment on the guest aspect...

Anyway, thanks for the initial run-thru - I'll keep coming back to this
as time provides and we can talk.

Others reading are ofc more than welcome to do patches...

;-)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette