| Message ID | 20190814183103.33707-2-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: E0PD support | expand |
Hi Mark, On Wed, Aug 14, 2019 at 07:31:02PM +0100, Mark Brown wrote: > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 9323bcc40a58..62b01fc35ef6 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -219,6 +219,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = { > }; > > static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_E0PD_SHIFT, 4, 0), The presence of this entry means the system-wide value will be exposed to a KVM guest, but since it's NONSTRICT we won't warn about a late-onlined CPU that has a mismatch. So if all the boot-time CPUs have E0PD, we can spawn a VM that starts using E0PD, but we might (silently) later migrate it to a CPU without E0PD, breaking the security guarantee. I think we want this to be STRICT, so that we at least warn in such a case. More generally than this patch, I suspect we probably want to abort the hotplug if we online a CPU that doesn't provide the same gaurantees as the sys_val for the field. [...] > @@ -1559,6 +1573,19 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .sign = FTR_UNSIGNED, > .min_field_value = 1, > }, > +#endif > +#ifdef CONFIG_ARM64_E0PD > + { > + .desc = "E0PD", > + .capability = ARM64_HAS_E0PD, > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, I suspect it would be better to treat this as a system-wide capability, as with KPTI, which will make it much easier to reason about. That would rule out having E0PD on a subset of CPUs, with or without KPTI. With KPTI it's not really necessary, and without KPTI we don't have a consistent guarantee, so that sounds reasonable to me. Thanks, Mark. > + .sys_reg = SYS_ID_AA64MMFR2_EL1, > + .sign = FTR_UNSIGNED, > + .field_pos = ID_AA64MMFR2_E0PD_SHIFT, > + .matches = has_cpuid_feature, > + .min_field_value = 1, > + .cpu_enable = cpu_enable_e0pd, > + }, > #endif > {}, > }; > -- > 2.20.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Oct 10, 2019 at 05:13:17PM +0100, Mark Rutland wrote: > So if all the boot-time CPUs have E0PD, we can spawn a VM that starts > using E0PD, but we might (silently) later migrate it to a CPU without > E0PD, breaking the security guarantee. > I think we want this to be STRICT, so that we at least warn in such a > case. > More generally than this patch, I suspect we probably want to abort the > hotplug if we online a CPU that doesn't provide the same gaurantees as > the sys_val for the field. Right, if we make it STRICT we at least avoid that issue with KVM. > > +#ifdef CONFIG_ARM64_E0PD > > + { > > + .desc = "E0PD", > > + .capability = ARM64_HAS_E0PD, > > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > I suspect it would be better to treat this as a system-wide capability, > as with KPTI, which will make it much easier to reason about. > That would rule out having E0PD on a subset of CPUs, with or without > KPTI. With KPTI it's not really necessary, and without KPTI we don't > have a consistent guarantee, so that sounds reasonable to me. It does - the main motivation for doing it as a local feature was to avoid the regression with systems with late CPUs that lack the capability which Will was concerned about but I'm not sure how realistic such systems actually are.
On Fri, Oct 11, 2019 at 12:17:15PM +0100, Mark Brown wrote: > On Thu, Oct 10, 2019 at 05:13:17PM +0100, Mark Rutland wrote: > > > So if all the boot-time CPUs have E0PD, we can spawn a VM that starts > > using E0PD, but we might (silently) later migrate it to a CPU without > > E0PD, breaking the security guarantee. > > > I think we want this to be STRICT, so that we at least warn in such a > > case. > > > More generally than this patch, I suspect we probably want to abort the > > hotplug if we online a CPU that doesn't provide the same gaurantees as > > the sys_val for the field. > > Right, if we make it STRICT we at least avoid that issue with KVM. > > > > +#ifdef CONFIG_ARM64_E0PD > > > + { > > > + .desc = "E0PD", > > > + .capability = ARM64_HAS_E0PD, > > > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > > > I suspect it would be better to treat this as a system-wide capability, > > as with KPTI, which will make it much easier to reason about. > > > That would rule out having E0PD on a subset of CPUs, with or without > > KPTI. With KPTI it's not really necessary, and without KPTI we don't > > have a consistent guarantee, so that sounds reasonable to me. > > It does - the main motivation for doing it as a local feature was > to avoid the regression with systems with late CPUs that lack the > capability which Will was concerned about but I'm not sure how > realistic such systems actually are. I think we need to handle the case where not all CPUs support ExPD :( It's not like E0PD is something critical like support for a particular page size, so I think it's a tough sell for SoC integrators to ensure that it's matched up. Will
On Fri, Oct 11, 2019 at 12:40:13PM +0100, Will Deacon wrote: > On Fri, Oct 11, 2019 at 12:17:15PM +0100, Mark Brown wrote: > > On Thu, Oct 10, 2019 at 05:13:17PM +0100, Mark Rutland wrote: > > > > > So if all the boot-time CPUs have E0PD, we can spawn a VM that starts > > > using E0PD, but we might (silently) later migrate it to a CPU without > > > E0PD, breaking the security guarantee. > > > > > I think we want this to be STRICT, so that we at least warn in such a > > > case. > > > > > More generally than this patch, I suspect we probably want to abort the > > > hotplug if we online a CPU that doesn't provide the same gaurantees as > > > the sys_val for the field. > > > > Right, if we make it STRICT we at least avoid that issue with KVM. > > > > > > +#ifdef CONFIG_ARM64_E0PD > > > > + { > > > > + .desc = "E0PD", > > > > + .capability = ARM64_HAS_E0PD, > > > > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > > > > > I suspect it would be better to treat this as a system-wide capability, > > > as with KPTI, which will make it much easier to reason about. > > > > > That would rule out having E0PD on a subset of CPUs, with or without > > > KPTI. With KPTI it's not really necessary, and without KPTI we don't > > > have a consistent guarantee, so that sounds reasonable to me. > > > > It does - the main motivation for doing it as a local feature was > > to avoid the regression with systems with late CPUs that lack the > > capability which Will was concerned about but I'm not sure how > > realistic such systems actually are. > > I think we need to handle the case where not all CPUs support ExPD :( I think we'll have to handle systems with mixed support, but: * We should only make use E0PD the feature when all the boot CPUs have it. * We should reject late-onlining of a CPU without E0PD if we detected it at boot time and either used it or expose it to guests. If people are late-onlining mismatched secondaries they get to pick up the pieces, as with other features. Thanks, Mark.
On Fri, Oct 11, 2019 at 12:40:13PM +0100, Will Deacon wrote: > On Fri, Oct 11, 2019 at 12:17:15PM +0100, Mark Brown wrote: > > On Thu, Oct 10, 2019 at 05:13:17PM +0100, Mark Rutland wrote: > > > > > So if all the boot-time CPUs have E0PD, we can spawn a VM that starts > > > using E0PD, but we might (silently) later migrate it to a CPU without > > > E0PD, breaking the security guarantee. > > > > > I think we want this to be STRICT, so that we at least warn in such a > > > case. > > > > > More generally than this patch, I suspect we probably want to abort the > > > hotplug if we online a CPU that doesn't provide the same gaurantees as > > > the sys_val for the field. > > > > Right, if we make it STRICT we at least avoid that issue with KVM. > > > > > > +#ifdef CONFIG_ARM64_E0PD > > > > + { > > > > + .desc = "E0PD", > > > > + .capability = ARM64_HAS_E0PD, > > > > + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, > > > > > I suspect it would be better to treat this as a system-wide capability, > > > as with KPTI, which will make it much easier to reason about. > > > > > That would rule out having E0PD on a subset of CPUs, with or without > > > KPTI. With KPTI it's not really necessary, and without KPTI we don't > > > have a consistent guarantee, so that sounds reasonable to me. > > > > It does - the main motivation for doing it as a local feature was > > to avoid the regression with systems with late CPUs that lack the > > capability which Will was concerned about but I'm not sure how > > realistic such systems actually are. > > I think we need to handle the case where not all CPUs support ExPD :( That's fine but if such CPUs are brought up late and the kernel has already made the decision to switch to global mappings, we should just park them.
On Fri, Oct 11, 2019 at 12:40:13PM +0100, Will Deacon wrote: > On Fri, Oct 11, 2019 at 12:17:15PM +0100, Mark Brown wrote: > > It does - the main motivation for doing it as a local feature was > > to avoid the regression with systems with late CPUs that lack the > > capability which Will was concerned about but I'm not sure how > > realistic such systems actually are. > I think we need to handle the case where not all CPUs support ExPD :( > It's not like E0PD is something critical like support for a particular page > size, so I think it's a tough sell for SoC integrators to ensure that it's > matched up. That's fine, we handle them by falling back to KPTI if we detect any non-E0PD CPUs. The only difference in the end result between doing this by making it a system wide capability and doing it as a weak local capability is what happens if the first CPU that appears without E0PD is a late CPU then instead of accepting the CPU and enabling KPTI we will reject the CPU.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 12de5c6075ec..7bf403405c99 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1392,6 +1392,21 @@ config ARM64_PTR_AUTH endmenu +menu "ARMv8.5 architectural features" + +config ARM64_E0PD + bool "Enable support for E0PD" + default y + help + E0PD (part of the ARMv8.5 extensions) allows us to ensure + that EL0 accesses made via TTBR1 always fault in constant time, + providing benefits to KPTI with lower overhead and without + disrupting legitimate access to kernel memory such as SPE. + + This option enables E0PD for TTBR1 where available. + +endmenu + config ARM64_SVE bool "ARM Scalable Vector Extension support" default y diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index f19fe4b9acc4..f25388981075 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -52,7 +52,8 @@ #define ARM64_HAS_IRQ_PRIO_MASKING 42 #define ARM64_HAS_DCPODP 43 #define ARM64_WORKAROUND_1463225 44 +#define ARM64_HAS_E0PD 45 -#define ARM64_NCAPS 45 +#define ARM64_NCAPS 46 #endif /* __ASM_CPUCAPS_H */ diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h index db92950bb1a0..1a2708ebcae8 100644 --- a/arch/arm64/include/asm/pgtable-hwdef.h +++ b/arch/arm64/include/asm/pgtable-hwdef.h @@ -292,6 +292,8 @@ #define TCR_HD (UL(1) << 40) #define TCR_NFD0 (UL(1) << 53) #define TCR_NFD1 (UL(1) << 54) +#define TCR_E0PD0 (UL(1) << 55) +#define TCR_E0PD1 (UL(1) << 56) /* * TTBR. diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 1df45c7ffcf7..37a0926536d3 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -652,6 +652,7 @@ #define ID_AA64MMFR1_VMIDBITS_16 2 /* id_aa64mmfr2 */ +#define ID_AA64MMFR2_E0PD_SHIFT 60 #define ID_AA64MMFR2_FWB_SHIFT 40 #define ID_AA64MMFR2_AT_SHIFT 32 #define ID_AA64MMFR2_LVA_SHIFT 16 diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 9323bcc40a58..62b01fc35ef6 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -219,6 +219,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr1[] = { }; static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { + ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_E0PD_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0), @@ -1244,6 +1245,19 @@ static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap) } #endif /* CONFIG_ARM64_PTR_AUTH */ +#ifdef CONFIG_ARM64_E0PD +static void cpu_enable_e0pd(struct arm64_cpu_capabilities const *cap) +{ + /* + * The cpu_enable() callback gets called even on CPUs that + * don't detect the feature so we need to verify if we can + * enable. + */ + if (this_cpu_has_cap(ARM64_HAS_E0PD)) + sysreg_clear_set(tcr_el1, 0, TCR_E0PD1); +} +#endif /* CONFIG_ARM64_E0PD */ + #ifdef CONFIG_ARM64_PSEUDO_NMI static bool enable_pseudo_nmi; @@ -1559,6 +1573,19 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .sign = FTR_UNSIGNED, .min_field_value = 1, }, +#endif +#ifdef CONFIG_ARM64_E0PD + { + .desc = "E0PD", + .capability = ARM64_HAS_E0PD, + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .sys_reg = SYS_ID_AA64MMFR2_EL1, + .sign = FTR_UNSIGNED, + .field_pos = ID_AA64MMFR2_E0PD_SHIFT, + .matches = has_cpuid_feature, + .min_field_value = 1, + .cpu_enable = cpu_enable_e0pd, + }, #endif {}, };