| 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: Sat, 09 May 2020 11:59:07 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
James Morse <james.morse@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
kvmarm@...ts.cs.columbia.edu, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] arm64/cpufeature: Validate hypervisor capabilities during CPU hotplug
On Fri, 08 May 2020 03:52:38 +0100,
Anshuman Khandual <anshuman.khandual@....com> wrote:
>
> This validates hypervisor capabilities like VMID width, IPA range for any
> hot plug CPU against system finalized values. While here, it factors out
> get_vmid_bits() for general use and also defines ID_AA64MMFR0_PARANGE_MASK.
Maybe add a quick word on the fact that we use KVM's view of the IPA
space to allow a CPU to come up.
>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Marc Zyngier <maz@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: James Morse <james.morse@....com>
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: kvmarm@...ts.cs.columbia.edu
> Cc: linux-kernel@...r.kernel.org
>
> Suggested-by: Suzuki Poulose <suzuki.poulose@....com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
> Changes in V2:
>
> - Added is_hyp_mode_available() check per Marc
> - Moved verify_kvm_capabilities() into cpufeature.c per Marc
> - Added helper get_kvm_ipa_limit() to fetch kvm_ipa_limit per Marc
> - Renamed kvm as hyp including the commit message per Marc
>
> Changes in V1: (https://patchwork.kernel.org/patch/11532565/)
>
> arch/arm64/include/asm/cpufeature.h | 20 +++++++++++++++++
> arch/arm64/include/asm/kvm_mmu.h | 2 +-
> arch/arm64/include/asm/sysreg.h | 1 +
> arch/arm64/kernel/cpufeature.c | 33 +++++++++++++++++++++++++++++
> arch/arm64/kvm/reset.c | 11 ++++++++--
> 5 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index afe08251ff95..fbbb4d2216f0 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -745,6 +745,26 @@ static inline bool cpu_has_hw_af(void)
> extern bool cpu_has_amu_feat(int cpu);
> #endif
>
> +static inline unsigned int get_vmid_bits(u64 mmfr1)
> +{
> + int vmid_bits;
> +
> + vmid_bits = cpuid_feature_extract_unsigned_field(mmfr1,
> + ID_AA64MMFR1_VMIDBITS_SHIFT);
> + if (vmid_bits == ID_AA64MMFR1_VMIDBITS_16)
> + return 16;
> +
> + /*
> + * Return the default here even if any reserved
> + * value is fetched from the system register.
> + */
> + return 8;
> +}
> +
> +#ifdef CONFIG_KVM_ARM_HOST
nit: useless #ifdefery.
> +u32 get_kvm_ipa_limit(void);
> +#endif
> +
> #endif /* __ASSEMBLY__ */
>
> #endif
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 30b0e8d6b895..a7137e144b97 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -416,7 +416,7 @@ static inline unsigned int kvm_get_vmid_bits(void)
> {
> int reg = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
>
> - return (cpuid_feature_extract_unsigned_field(reg, ID_AA64MMFR1_VMIDBITS_SHIFT) == 2) ? 16 : 8;
> + return get_vmid_bits(reg);
> }
>
> /*
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index c4ac0ac25a00..3510a4668970 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -705,6 +705,7 @@
> #define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> #define ID_AA64MMFR0_PARANGE_48 0x5
> #define ID_AA64MMFR0_PARANGE_52 0x6
> +#define ID_AA64MMFR0_PARANGE_MASK 0x7
I realise this is already like this in the current code, but using 7
as a mask value for the feature feels wrong. If we ever get a value
with bit 3 of the capability being set, we will confuse it with some
other configuration.
We should be more careful and pass the full value of the feature to
id_aa64mmfr0_parange_to_phys_shift(), which already does the right
thing.
>
> #ifdef CONFIG_ARM64_PA_BITS_52
> #define ID_AA64MMFR0_PARANGE_MAX ID_AA64MMFR0_PARANGE_52
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 9fac745aa7bb..7e5ff452574c 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2181,6 +2181,36 @@ static void verify_sve_features(void)
> /* Add checks on other ZCR bits here if necessary */
> }
>
> +#ifdef CONFIG_KVM_ARM_HOST
> +void verify_hyp_capabilities(void)
> +{
> + u64 safe_mmfr1, mmfr0, mmfr1;
> + int parange, ipa_max;
> + unsigned int safe_vmid_bits, vmid_bits;
> +
> + safe_mmfr1 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> + mmfr0 = read_cpuid(ID_AA64MMFR0_EL1);
> + mmfr1 = read_cpuid(ID_AA64MMFR1_EL1);
> +
> + /* Verify VMID bits */
> + safe_vmid_bits = get_vmid_bits(safe_mmfr1);
> + vmid_bits = get_vmid_bits(mmfr1);
> + if (vmid_bits < safe_vmid_bits) {
> + pr_crit("CPU%d: VMID width mismatch\n", smp_processor_id());
> + cpu_die_early();
> + }
> +
> + /* Verify IPA range */
> + parange = mmfr0 & ID_AA64MMFR0_PARANGE_MASK;
> + ipa_max = id_aa64mmfr0_parange_to_phys_shift(parange);
> + if (ipa_max < get_kvm_ipa_limit()) {
> + pr_crit("CPU%d: IPA range mismatch\n", smp_processor_id());
> + cpu_die_early();
> + }
> +}
> +#else /* !CONFIG_KVM_ARM_HOST */
> +static inline void verify_hyp_capabilities(void) { }
> +#endif /* CONFIG_KVM_ARM_HOST */
>
> /*
> * Run through the enabled system capabilities and enable() it on this CPU.
> @@ -2206,6 +2236,9 @@ static void verify_local_cpu_capabilities(void)
>
> if (system_supports_sve())
> verify_sve_features();
> +
> + if (is_hyp_mode_available())
> + verify_hyp_capabilities();
> }
>
> void check_local_cpu_capabilities(void)
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 30b7ea680f66..1131b112dda2 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -340,11 +340,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> return ret;
> }
>
> +u32 get_kvm_ipa_limit(void)
> +{
> + return kvm_ipa_limit;
> +}
> +
> void kvm_set_ipa_limit(void)
> {
> unsigned int ipa_max, pa_max, va_max, parange;
>
> - parange = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1) & 0x7;
> + parange = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1) &
> + ID_AA64MMFR0_PARANGE_MASK;
> pa_max = id_aa64mmfr0_parange_to_phys_shift(parange);
>
> /* Clamp the IPA limit to the PA size supported by the kernel */
> @@ -406,7 +412,8 @@ int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
> phys_shift = KVM_PHYS_SHIFT;
> }
>
> - parange = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1) & 7;
> + parange = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1) &
> + ID_AA64MMFR0_PARANGE_MASK;
> if (parange > ID_AA64MMFR0_PARANGE_MAX)
> parange = ID_AA64MMFR0_PARANGE_MAX;
> vtcr |= parange << VTCR_EL2_PS_SHIFT;
> --
> 2.20.1
>
>
With the couple of nits above addressed:
Reviewed-by: Marc Zyngier <maz@...nel.org>
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists