| 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, 11 Jan 2021 19:48:14 +0000
From: Marc Zyngier <maz@...nel.org>
To: Catalin Marinas <catalin.marinas@....com>
Cc: linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
David Brazdil <dbrazdil@...gle.com>,
Alexandru Elisei <alexandru.elisei@....com>,
Ard Biesheuvel <ardb@...nel.org>,
Jing Zhang <jingzhangos@...gle.com>,
Ajay Patil <pajay@....qualcomm.com>,
Prasad Sodagudi <psodagud@...eaurora.org>,
Srinivas Ramana <sramana@...eaurora.org>,
James Morse <james.morse@....com>,
Julien Thierry <julien.thierry.kdev@...il.com>,
Suzuki K Poulose <suzuki.poulose@....com>,
kernel-team@...roid.com
Subject: Re: [PATCH v3 09/21] arm64: cpufeature: Add global feature override
facility
Hi Catalin,
On 2021-01-11 18:41, Catalin Marinas wrote:
> Hi Marc,
>
> On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
>> Add a facility to globally override a feature, no matter what
>> the HW says. Yes, this is dangerous.
>
> Yeah, it's dangerous. We can make it less so if we only allow safe
> values (e.g. lower if FTR_UNSIGNED).
My plan was also to allow non-safe values in order to trigger features
that are not advertised by the HW. But I can understand if you are
reluctant to allow such thing! :D
>> diff --git a/arch/arm64/include/asm/cpufeature.h
>> b/arch/arm64/include/asm/cpufeature.h
>> index 9a555809b89c..465d2cb63bfc 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
>> u64 sys_val;
>> u64 user_val;
>> const struct arm64_ftr_bits *ftr_bits;
>> + u64 *override_val;
>> + u64 *override_mask;
>> };
>
> At the arm64_ftr_reg level, we don't have any information about the
> safe
> values for a feature. Could we instead move this to arm64_ftr_bits? We
> probably only need a single field. When populating the feature values,
> we can make sure it doesn't go above the hardware one.
>
> I attempted a feature modification for MTE here, though I dropped the
> entire series in the meantime as we clarified the ARM ARM:
>
> https://lore.kernel.org/linux-arm-kernel/20200515171612.1020-24-catalin.marinas@arm.com/
>
> Srinivas copied it in his patch (but forgot to give credit ;)):
>
> https://lore.kernel.org/linux-arm-msm/1610152163-16554-3-git-send-email-sramana@codeaurora.org/
>
> The above adds a filter function but, instead, just use your mechanism
> in
> this series for idreg.feature setting via cmdline. The
> arm64_ftr_value()
> function extracts the hardware value and lowers it if a cmdline
> argument
> was passed.
One thing is that it is not always possible to sanitise the value passed
if it is required very early on, as I do with VHE. But in that case
I actually check that we are VHE capable before starting to poke at
VHE-specific state.
I came up with the following patch on top, which preserves the current
global approach (no per arm64_ftr_bits state), but checks (and alters)
the override as it iterates through the various fields.
For example, if I pass "arm64.nopauth kvm-arm.mode=nvhe
id_aa64pfr1.bt=5"
to the FVP, I get the following output:
[ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[31:28]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[11:8]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64MMFR1_EL1[11:8]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64PFR1_EL1[3:0]: not forcing 1 to
5
[ 0.000000] CPU features: detected: GIC system register CPU interface
[ 0.000000] CPU features: detected: Hardware dirty bit management
[ 0.000000] CPU features: detected: Spectre-v4
[ 0.000000] CPU features: detected: Branch Target Identification
showing that the PAC features have been downgraded, together with VHE,
but that BTI is still detected as value 5 was obviously bogus.
Thoughts?
M.
diff --git a/arch/arm64/kernel/cpufeature.c
b/arch/arm64/kernel/cpufeature.c
index 894af60b9669..00d99e593b65 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64
new)
u64 strict_mask = ~0x0ULL;
u64 user_mask = 0;
u64 valid_mask = 0;
+ u64 override_val = 0, override_mask = 0;
const struct arm64_ftr_bits *ftrp;
struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
@@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
u64 new)
if (!reg)
return;
+ if (reg->override_mask && reg->override_val) {
+ override_mask = *reg->override_mask;
+ override_val = *reg->override_val;
+ }
+
for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
u64 ftr_mask = arm64_ftr_mask(ftrp);
s64 ftr_new = arm64_ftr_value(ftrp, new);
+ s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
+
+ if ((ftr_mask & override_mask) == ftr_mask) {
+ if (ftr_ovr < ftr_new) {
+ pr_warn("%s[%d:%d]: forced from %llx to %llx\n",
+ reg->name,
+ ftrp->shift + ftrp->width - 1,
+ ftrp->shift, ftr_new, ftr_ovr);
+
+ ftr_new = ftr_ovr;
+ } else if (ftr_ovr != ftr_new) {
+ pr_warn("%s[%d:%d]: not forcing %llx to %llx\n",
+ reg->name,
+ ftrp->shift + ftrp->width - 1,
+ ftrp->shift, ftr_new, ftr_ovr);
+
+ /* Remove the override */
+ *reg->override_mask &= ~ftr_mask;
+ *reg->override_val &= ~ftr_mask;
+ }
+ }
val = arm64_ftr_set_value(ftrp, val, ftr_new);
@@ -800,18 +827,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
u64 new)
val &= valid_mask;
- if (reg->override_mask && reg->override_val) {
- u64 override = val;
- override &= ~*reg->override_mask;
- override |= (*reg->override_val & *reg->override_mask);
-
- if (val != override)
- pr_warn("%s: forced from %016llx to %016llx\n",
- reg->name, val, override);
-
- val = override;
- }
-
reg->sys_val = val;
reg->strict_mask = strict_mask;
reg->user_mask = user_mask;
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists