| 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
| ||
|
Date: Mon, 27 Feb 2023 08:13:42 +0100
From: Juergen Gross <jgross@...e.com>
To: "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v3 03/12] x86/mtrr: support setting MTRR state for
software defined MTRRs
On 26.02.23 18:12, Michael Kelley (LINUX) wrote:
> From: Juergen Gross <jgross@...e.com> Sent: Thursday, February 23, 2023 1:33 AM
>>
>> 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.
>>
>> Signed-off-by: Juergen Gross <jgross@...e.com>
>> ---
>> V2:
>> - new patch
>> V3:
>> - omit fixed MTRRs, as those are currently not needed
>> - disable X86_FEATURE_MTRR instead of testing it
>> - provide a stub for !CONFIG_MTRR (Michael Kelley)
>> - use cpu_feature_enabled() (Boris Petkov)
>> - add tests for mtrr_overwrite_state() being allowed (Boris Petkov)
>> ---
>> arch/x86/include/asm/mtrr.h | 8 ++++++
>> arch/x86/kernel/cpu/mtrr/generic.c | 43 ++++++++++++++++++++++++++++++
>> arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +++++++
>> arch/x86/kernel/setup.c | 2 ++
>> 4 files changed, 62 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
>> index f0eeaf6e5f5f..f1cb81330a64 100644
>> --- a/arch/x86/include/asm/mtrr.h
>> +++ b/arch/x86/include/asm/mtrr.h
>> @@ -31,6 +31,8 @@
>> */
>> # ifdef CONFIG_MTRR
>> void mtrr_bp_init(void);
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> + mtrr_type def_type);
>> extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
>> extern void mtrr_save_fixed_ranges(void *);
>> extern void mtrr_save_state(void);
>> @@ -48,6 +50,12 @@ void mtrr_disable(void);
>> void mtrr_enable(void);
>> void mtrr_generic_set_state(void);
>> # else
>> +static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
>> + unsigned int num_var,
>> + mtrr_type def_type)
>> +{
>> +}
>> +
>> static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
>> {
>> /*
>> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
>> index ee09d359e08f..40c59d522f57 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/tlbflush.h>
>> #include <asm/mtrr.h>
>> #include <asm/msr.h>
>> @@ -240,6 +242,47 @@ 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).
>> + * Is allowed only for special cases when running virtualized. Must be called
>> + * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
>> + */
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> + mtrr_type def_type)
>> +{
>> + unsigned int i;
>> +
>> + if (WARN_ON(mtrr_state_set ||
>> + hypervisor_is_type(X86_HYPER_NATIVE) ||
>> + !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
>> + (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
>
> With current upstream code, this test doesn't allow Hyper-V SNP vTOM VMs
> to do the overwrite, as current upstream vTOM code doesn't participate in the
> cc_platform_has() mechanism. That's being reworked in a separate patch
> set. Can you add this test to cover the SNP vTOM case?
>
> !hv_is_isolation_supported() &&
>
> This is the same test used in __set_memory_enc_dec(), for example. You'll
> have to add #include <asm/mshyperv.h>. There's already a stub that returns
> 'false' so that everything works when building with CONFIG_HYPERV=n.
Yes, this was an open question. In case nobody objects, I'll do the
modification.
> Once my other patch set is accepted, I'll revise this to remove use of
> hv_is_isolation_supported() outside of Hyper-V specific code, and use
> the newer cc_platform_has() instead.
Thanks,
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists