linux-kernel - Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

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: <f9511025-f815-c8fa-f6e7-80501e8c839f@suse.com>
Date:   Mon, 20 Mar 2023 14:47:30 +0100
From:   Juergen Gross <jgross@...e.com>
To:     "Huang, Kai" <kai.huang@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
Cc:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for
 software defined MTRRs

On 20.03.23 13:59, Huang, Kai wrote:
> On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
>> When running virtualized, MTRR access can be reduced (e.g. in Xen PV
>> guests or when running as a SEV-SNP guest under Hyper-V). Typically
>> the hypervisor will reset the MTRR feature in CPUID data, resulting
>> in no MTRR memory type information being available for the kernel.
>>
>> This has turned out to result in problems:
>>
>> - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
>> - Xen PV dom0 mapping memory as WB which should be UC- instead
>>
>> Solve those problems by supporting to set a static MTRR state,
>> overwriting the empty state used today. In case such a state has been
>> set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
>> will only be used by mtrr_type_lookup(), as in all other cases
>> mtrr_enabled() is being checked, which will return false. Accept the
>> overwrite call only for selected cases when running as a guest.
>> Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
>> just refusing them.
>>
>>
> [...]
> 
>> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
>> index ee09d359e08f..49b4cc923312 100644
>> --- a/arch/x86/kernel/cpu/mtrr/generic.c
>> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
>> @@ -8,10 +8,12 @@
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/mm.h>
>> -
>> +#include <linux/cc_platform.h>
>>   #include <asm/processor-flags.h>
>>   #include <asm/cacheinfo.h>
>>   #include <asm/cpufeature.h>
>> +#include <asm/hypervisor.h>
>> +#include <asm/mshyperv.h>
> 
> Is <asm/mshyperv.h> needed here?

Yes, for hv_is_isolation_supported().

> 
>>   #include <asm/tlbflush.h>
>>   #include <asm/mtrr.h>
>>   #include <asm/msr.h>
>> @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>>   	return mtrr_state.def_type;
>>   }
>>   
>> +/**
>> + * mtrr_overwrite_state - set static MTRR state
>> + *
>> + * Used to set MTRR state via different means (e.g. with data obtained from
>> + * a hypervisor).
> 
> +KVM list and KVM maintainers,
> 
> IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
> hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
> because they have mutual understanding?
> 
> What about the SNP guest running on other hypervisors such as KVM?
> 
> Since this code covers TDX guest too, I think eventually it makes sense for TDX
> guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> as I am not sure what's the point of making the TDX guest's MTRR behaviour
> depending on specific hypervisor.

This series tries to fix the current fallout.

Boris Petkov asked for the hypervisor specific tests to be added, so I've
added them after discussing the topic with him (he is the maintainer of
this code after all).

> For now I don't see there's any use case for TDX guest to use non-WB memory type
> (in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
> the guest memory), so to me it seems it's OK to make a universal mutual
> understanding that TDX guest will always have WB memory type for all memory.

I agree.

> But, I am not sure whether it's better to have a standard hypercall between
> guest & hypervisor for this purpose so things can be more flexible?

Maybe. But for now we need to handle the current situation.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)