| 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
| ||
|
Message-ID: <20170622092859.GA3714@earth.li>
Date: Thu, 22 Jun 2017 10:28:59 +0100
From: Jonathan McDowell <noodles@...th.li>
To: andi@...stfloor.org
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies
In article <20170621234106.16548-3-andi@...stfloor.org> you wrote:
> Some CPUID features depend on other features. Currently it's
> possible to to clear dependent features, but not clear the base features,
> which can cause various interesting problems.
> This patch implements a generic table to describe dependencies
> between CPUID features, to be used by all code that clears
> CPUID.
> Some subsystems (like XSAVE) had an own implementation of this,
> but it's better to do it all in a single place for everyone.
> Then clear_cpu_cap and setup_clear_cpu_cap always look up
> this table and clear all dependencies too.
> This is intended to be a practical table: only for features
> that make sense to clear. If someone for example clears FPU,
> or other features that are essentially part of the required
> base feature set, not much is going to work. Handling
> that is right now out of scope. We're only handling
> features which can be usefully cleared.
> v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> ---
> arch/x86/include/asm/cpufeature.h | 8 +++-
> arch/x86/include/asm/cpufeatures.h | 5 +++
> arch/x86/kernel/cpu/Makefile | 1 +
> arch/x86/kernel/cpu/cpuid-deps.c | 92 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 104 insertions(+), 2 deletions(-)
> create mode 100644 arch/x86/kernel/cpu/cpuid-deps.c
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index d59c15c3defd..e6145f383ff8 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -125,8 +125,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> #define boot_cpu_has(bit) cpu_has(&boot_cpu_data, bit)
>
> #define set_cpu_cap(c, bit) set_bit(bit, (unsigned long *)((c)->x86_capability))
> -#define clear_cpu_cap(c, bit) clear_bit(bit, (unsigned long *)((c)->x86_capability))
> -#define setup_clear_cpu_cap(bit) do { \
> +#define __clear_cpu_cap(c, bit) clear_bit(bit, (unsigned long *)((c)->x86_capability))
> +
> +extern void setup_clear_cpu_cap(int bit);
> +extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit);
> +
> +#define __setup_clear_cpu_cap(bit) do { \
> clear_cpu_cap(&boot_cpu_data, bit); \
> set_bit(bit, (unsigned long *)cpu_caps_cleared); \
> } while (0)
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 2701e5f8145b..8f371a5966e7 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -21,6 +21,11 @@
> * this feature bit is not displayed in /proc/cpuinfo at all.
> */
>
> +/*
> + * When adding new features here that depend on other features,
> + * please update the table in kernel/cpu/cpuid-deps.c
> + */
> +
> /* Intel-defined CPU features, CPUID level 0x00000001 (edx), word 0 */
> #define X86_FEATURE_FPU ( 0*32+ 0) /* Onboard FPU */
> #define X86_FEATURE_VME ( 0*32+ 1) /* Virtual Mode Extensions */
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 52000010c62e..274fc0fee1e1 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -21,6 +21,7 @@ obj-y += common.o
> obj-y += rdrand.o
> obj-y += match.o
> obj-y += bugs.o
> +obj-y += cpuid-deps.o
>
> obj-$(CONFIG_PROC_FS) += proc.o
> obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> new file mode 100644
> index 000000000000..08aff02cf2ff
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,92 @@
> +/* Declare dependencies between CPUIDs */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <asm/cpufeature.h>
> +
> +struct cpuid_dep {
> + int feature;
> + int dep;
> +};
This seems a little confusing; I had to read through a couple of times
to understand that "dep" represents the feature that needs to be
disabled if "feature" is disabled, rather than dep being a dependency
of feature.
> +
> +/*
> + * Table of CPUID features that depend on others.
> + *
> + * This only includes dependencies that can be usefully disabled, not
> + * features part of the base set (like FPU).
> + */
> +const static struct cpuid_dep cpuid_deps[] = {
> + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVEOPT },
> + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVEC },
> + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVES },
> + { X86_FEATURE_XSAVE, X86_FEATURE_AVX },
> + { X86_FEATURE_XSAVE, X86_FEATURE_AVX512F },
> + { X86_FEATURE_XSAVE, X86_FEATURE_PKU },
> + { X86_FEATURE_XSAVE, X86_FEATURE_MPX },
> + { X86_FEATURE_XSAVE, X86_FEATURE_XGETBV1 },
> + { X86_FEATURE_XMM, X86_FEATURE_XMM2 },
> + { X86_FEATURE_XMM2, X86_FEATURE_XMM3 },
> + { X86_FEATURE_XMM2, X86_FEATURE_XMM4_1 },
> + { X86_FEATURE_XMM2, X86_FEATURE_XMM4_2 },
> + { X86_FEATURE_XMM2, X86_FEATURE_XMM3 },
> + { X86_FEATURE_XMM2, X86_FEATURE_PCLMULQDQ },
> + { X86_FEATURE_XMM2, X86_FEATURE_SSSE3 },
> + { X86_FEATURE_FMA, X86_FEATURE_AVX },
> + { X86_FEATURE_XMM2, X86_FEATURE_F16C },
> + { X86_FEATURE_XMM2, X86_FEATURE_AES },
> + { X86_FEATURE_XSAVE, X86_FEATURE_AVX },
> + { X86_FEATURE_XSAVE, X86_FEATURE_AVX512F },
Duplicates from above. (Might it be a better idea for the table to be
sorted to ease spotting such things and future patch merging?)
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512IFMA },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512PF },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512ER },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512CD },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512DQ },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512BW },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512VL },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512VBMI },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4VNNIW },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4FMAPS },
> + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_VPOPCNTDQ },
> + { X86_FEATURE_AVX, X86_FEATURE_AVX2 },
> + {}
> +};
> +
> +static void do_clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
> +{
> + int i, newfeat;
> + bool changed;
> +
> + if (!cpu)
> + __setup_clear_cpu_cap(feat);
> + else
> + __clear_cpu_cap(cpu, feat);
> +again:
> + changed = false;
> + for (i = 0; cpuid_deps[i].feature; i++) {
> + if (feat == cpuid_deps[i].feature) {
> + newfeat = cpuid_deps[i].dep;
> + if (!cpu)
> + __setup_clear_cpu_cap(newfeat);
> + else
> + __clear_cpu_cap(cpu, newfeat);
> + changed = true;
> + }
> + }
> + /* Handle multi-level dependencies */
> + if (changed) {
> + feat = newfeat;
> + goto again;
> + }
I don't think this works? You're only picking up the last newfeat to
process again. So if I disable X86_FEATURE_XSAVE the last newfeat will
be X86_FEATURE_AVX512F and the code won't correctly disable the features
which require X86_FEATURE_AVX?
> +}
> +
> +void clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
> +{
> + do_clear_cpu_cap(cpu, feat);
> +}
> +
> +EXPORT_SYMBOL_GPL(clear_cpu_cap);
> +
> +void setup_clear_cpu_cap(int feat)
> +{
> + do_clear_cpu_cap(NULL, feat);
> +}
J.
--
Web [ < fivemack> it is bruter-force than a really really stupid ]
site: http:// [ elephant [on his Python suduku solver] ] Made by
www.earth.li/~noodles/ [ ] HuggieTag 0.0.24
Powered by blists - more mailing lists