| 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: Thu, 20 Jun 2024 12:06:44 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
Oliver Upton <oliver.upton@...ux.dev>,
James Morse <james.morse@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Mark Brown <broonie@...nel.org>,
kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling
On Thu, 20 Jun 2024 07:58:07 +0100,
Anshuman Khandual <anshuman.khandual@....com> wrote:
>
> This adds new HDFGRTR2_GROUP and HDFGWTR2_GROUP groups in enum fgt_group_id
> for FGU handling managed with HDFGRTR2_EL2 and HDFGWTR2_EL2 registers.
>
> Cc: Marc Zyngier <maz@...nel.org>
> Cc: Oliver Upton <oliver.upton@...ux.dev>
> Cc: James Morse <james.morse@....com>
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: kvmarm@...ts.linux.dev
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
> arch/arm64/include/asm/kvm_arm.h | 8 +++++
> arch/arm64/include/asm/kvm_host.h | 2 ++
> arch/arm64/kvm/emulate-nested.c | 14 ++++++++
> arch/arm64/kvm/hyp/include/hyp/switch.h | 10 ++++++
> arch/arm64/kvm/nested.c | 36 ++++++++++++++++++++
> arch/arm64/kvm/sys_regs.c | 45 +++++++++++++++++++++++++
> 6 files changed, 115 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index b2adc2c6c82a..b3fb368bcadb 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -354,6 +354,14 @@
> #define __HFGRTR_EL2_MASK GENMASK(49, 0)
> #define __HFGRTR_EL2_nMASK ~(__HFGRTR_EL2_RES0 | __HFGRTR_EL2_MASK)
>
> +#define __HDFGRTR2_EL2_RES0 HDFGRTR2_EL2_RES0
> +#define __HDFGRTR2_EL2_MASK (GENMASK(22, 22) | GENMASK(20, 0))
How about bit 23? How comes you are considering all these bits as positive?
> +#define __HDFGRTR2_EL2_nMASK ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK)
Note the *nMASK* here. 'n' is for *negative*. Just look at how
__HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written.
> +
> +#define __HDFGWTR2_EL2_RES0 HDFGWTR2_EL2_RES0
> +#define __HDFGWTR2_EL2_MASK (GENMASK(22, 19) | GENMASK(16, 7) | GENMASK(5, 0))
Again, how about bit 23? Same remark for the polarity.
> +#define __HDFGWTR2_EL2_nMASK ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK)
> +
> /*
> * The HFGWTR bits are a subset of HFGRTR bits. To ensure we don't miss any
> * future additions, define __HFGWTR* macros relative to __HFGRTR* ones.
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7b44e96e7270..d6fbd6ebc32d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -239,6 +239,8 @@ enum fgt_group_id {
> HDFGWTR_GROUP = HDFGRTR_GROUP,
> HFGITR_GROUP,
> HAFGRTR_GROUP,
> + HDFGRTR2_GROUP,
> + HDFGWTR2_GROUP = HDFGRTR2_GROUP,
>
> /* Must be last */
> __NR_FGT_GROUP_IDS__
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index 54090967a335..bc5ea1e60a0a 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -1724,6 +1724,9 @@ static const struct encoding_to_trap_config encoding_to_fgt[] __initconst = {
> SR_FGT(SYS_AMEVCNTR0_EL0(2), HAFGRTR, AMEVCNTR02_EL0, 1),
> SR_FGT(SYS_AMEVCNTR0_EL0(1), HAFGRTR, AMEVCNTR01_EL0, 1),
> SR_FGT(SYS_AMEVCNTR0_EL0(0), HAFGRTR, AMEVCNTR00_EL0, 1),
> +
> + /* HDFGRTR2_EL2 */
> + SR_FGT(SYS_MDSELR_EL1, HDFGRTR2, nMDSELR_EL1, 1),
No. See the 'n' prefix on this bit?
Also, where are all the other bits for the HDFRxTR2 registers?
> };
>
> static union trap_config get_trap_config(u32 sysreg)
> @@ -1979,6 +1982,10 @@ static bool check_fgt_bit(struct kvm *kvm, bool is_read,
> sr = is_read ? HDFGRTR_EL2 : HDFGWTR_EL2;
> break;
>
> + case HDFGRTR2_GROUP:
> + sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2;
> + break;
> +
> case HAFGRTR_GROUP:
> sr = HAFGRTR_EL2;
> break;
> @@ -2053,6 +2060,13 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)
> val = __vcpu_sys_reg(vcpu, HDFGWTR_EL2);
> break;
>
> + case HDFGRTR2_GROUP:
> + if (is_read)
> + val = __vcpu_sys_reg(vcpu, HDFGRTR2_EL2);
> + else
> + val = __vcpu_sys_reg(vcpu, HDFGWTR2_EL2);
> + break;
> +
> case HAFGRTR_GROUP:
> val = __vcpu_sys_reg(vcpu, HAFGRTR_EL2);
> break;
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 0c4de44534b7..b5944aa6d9c8 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -89,6 +89,10 @@ static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> case HDFGWTR_EL2: \
> id = HDFGRTR_GROUP; \
> break; \
> + case HDFGRTR2_EL2: \
> + case HDFGWTR2_EL2: \
> + id = HDFGRTR2_GROUP; \
> + break; \
> case HAFGRTR_EL2: \
> id = HAFGRTR_GROUP; \
> break; \
> @@ -160,6 +164,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu)
> CHECK_FGT_MASKS(HDFGWTR_EL2);
> CHECK_FGT_MASKS(HAFGRTR_EL2);
> CHECK_FGT_MASKS(HCRX_EL2);
> + CHECK_FGT_MASKS(HDFGRTR2_EL2);
> + CHECK_FGT_MASKS(HDFGWTR2_EL2);
>
> if (!cpus_have_final_cap(ARM64_HAS_FGT))
> return;
> @@ -171,6 +177,8 @@ static inline void __activate_traps_hfgxtr(struct kvm_vcpu *vcpu)
> update_fgt_traps(hctxt, vcpu, kvm, HFGITR_EL2);
> update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR_EL2);
> update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR_EL2);
> + update_fgt_traps(hctxt, vcpu, kvm, HDFGRTR2_EL2);
> + update_fgt_traps(hctxt, vcpu, kvm, HDFGWTR2_EL2);
>
> if (cpu_has_amu())
> update_fgt_traps(hctxt, vcpu, kvm, HAFGRTR_EL2);
> @@ -200,6 +208,8 @@ static inline void __deactivate_traps_hfgxtr(struct kvm_vcpu *vcpu)
> __deactivate_fgt(hctxt, vcpu, kvm, HFGITR_EL2);
> __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR_EL2);
> __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR_EL2);
> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGRTR2_EL2);
> + __deactivate_fgt(hctxt, vcpu, kvm, HDFGWTR2_EL2);
>
> if (cpu_has_amu())
> __deactivate_fgt(hctxt, vcpu, kvm, HAFGRTR_EL2);
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index bae8536cbf00..ebe4e3972fed 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -384,6 +384,42 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
> res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1;
> set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1);
>
> + /* HDFG[RW]TR2_EL2 */
> + res0 = res1 = 0;
> +
> + /* FEAT_TRBE_MPAM is not exposed to the guest */
> + res0 |= HDFGRTR2_EL2_nTRBMPAM_EL1;
No. We are moving away from hard-coded features, so you have to
explicitly check it.
> +
> + /* FEAT_SPE_FDS is not exposed to the guest */
> + res0 |= HDFGRTR2_EL2_nPMSDSFR_EL1;
Same thing.
> +
> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP))
> + res0 |= HDFGRTR2_EL2_nMDSTEPOP_EL1;
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP))
> + res0 |= HDFGRTR2_EL2_nTRCITECR_EL1;
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP))
> + res0 |= (HDFGRTR2_EL2_nSPMDEVAFF_EL1 | HDFGRTR2_EL2_nSPMID |
> + HDFGRTR2_EL2_nSPMSCR_EL1 | HDFGRTR2_EL2_nSPMACCESSR_EL1 |
> + HDFGRTR2_EL2_nSPMCR_EL0 | HDFGRTR2_EL2_nSPMOVS |
> + HDFGRTR2_EL2_nSPMINTEN | HDFGRTR2_EL2_nSPMSELR_EL0 |
> + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 | HDFGRTR2_EL2_nSPMEVCNTRn_EL0 |
> + HDFGRTR2_EL2_nPMSSCR_EL1 | HDFGRTR2_EL2_nPMSSDATA);
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
> + res0 |= HDFGRTR2_EL2_nMDSELR_EL1;
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9))
> + res0 |= HDFGRTR2_EL2_nPMUACR_EL1;
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP))
> + res0 |= HDFGRTR2_EL2_nPMICFILTR_EL0;
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP))
> + res0 |= HDFGRTR2_EL2_nPMICNTR_EL0;
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP))
> + res0 |= HDFGRTR2_EL2_nPMIAR_EL1;
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) &&
> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP))
> + res0 |= HDFGRTR2_EL2_nPMECR_EL1;
And all of that suffers from the same issue as in
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=next&id=eb9d53d4a949c6d6d7c9f130e537f6b5687fedf9
> + set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1);
> + set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1);
How about the HDFGxTR2_EL2_RES1 bits?
> +
> /* Reuse the bits from the read-side and add the write-specific stuff */
> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP))
> res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f921af014d0c..8029f408855d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
> kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
> HAFGRTR_EL2_RES1);
>
> + /* FEAT_TRBE_MPAM is not exposed to the guest */
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1);
Same thing about dynamic configuration.
But more importantly, you are disabling anything *but* MPAM. Does it
seem right to you?
> +
> + /* FEAT_SPE_FDS is not exposed to the guest */
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMSDSFR_EL1);
Same thing.
> +
> + if (!kvm_has_feat_enum(kvm, ID_AA64DFR2_EL1, STEP, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSTEPOP_EL1);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, ITE, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRCITECR_EL1);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nSPMDEVAFF_EL1 |
> + HDFGRTR2_EL2_nSPMID |
> + HDFGRTR2_EL2_nSPMSCR_EL1 |
> + HDFGRTR2_EL2_nSPMACCESSR_EL1 |
> + HDFGRTR2_EL2_nSPMCR_EL0 |
> + HDFGRTR2_EL2_nSPMOVS |
> + HDFGRTR2_EL2_nSPMINTEN |
> + HDFGRTR2_EL2_nSPMSELR_EL0 |
> + HDFGRTR2_EL2_nSPMEVTYPERn_EL0 |
> + HDFGRTR2_EL2_nSPMEVCNTRn_EL0 |
> + HDFGRTR2_EL2_nPMSSCR_EL1 |
> + HDFGRTR2_EL2_nPMSSDATA);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMUACR_EL1);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICFILTR_EL0);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, PMICNTR, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMICNTR_EL0);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, SEBEP, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMIAR_EL1);
> +
> + if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP) &&
> + !kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP))
> + kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nPMECR_EL1);
> +
Overall, this looks completely broken.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists