| 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: <86edb9sgy0.wl-maz@kernel.org>
Date: Sat, 13 Apr 2024 11:19:35 +0100
From: Marc Zyngier <maz@...nel.org>
To: Sebastian Ott <sebott@...hat.com>
Cc: linux-arm-kernel@...ts.infradead.org,
kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.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>
Subject: Re: [PATCH 3/4] KVM: arm64: add emulation for CTR_EL0 register
On Fri, 05 Apr 2024 13:01:07 +0100,
Sebastian Ott <sebott@...hat.com> wrote:
>
> CTR_EL0 is currently handled as an invariant register, thus
> guests will be presented with the host value of that register.
> Add emulation for CTR_EL0 based on a per VM value.
>
> When CTR_EL0 is changed the reset function for CLIDR_EL1 is
> called to make sure we present the guest with consistent
> register values.
Isn't that a change in the userspace ABI? You are now creating an
explicit ordering between the write to CTR_EL0 and the rest of the
cache hierarchy registers. It has the obvious capacity to lead to the
wrong result in a silent way...
>
> Signed-off-by: Sebastian Ott <sebott@...hat.com>
> ---
> arch/arm64/kvm/sys_regs.c | 72 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 4d29b1a0842d..b0ba292259f9 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1874,6 +1874,55 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> return true;
> }
>
> +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> + vcpu->kvm->arch.ctr_el0 = 0;
> + return kvm_get_ctr_el0(vcpu->kvm);
I'd expect the cached value to be reset instead of being set to
0. What are you achieving by this?
> +}
> +
> +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> + u64 *val)
> +{
> + *val = kvm_get_ctr_el0(vcpu->kvm);
> + return 0;
> +}
> +
> +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding);
> +
> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> + u64 val)
> +{
> + u64 host_val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + u64 old_val = kvm_get_ctr_el0(vcpu->kvm);
> + const struct sys_reg_desc *clidr_el1;
> + int ret;
> +
> + if (val == old_val)
> + return 0;
> +
> + if (kvm_vm_has_ran_once(vcpu->kvm))
> + return -EBUSY;
> +
> + mutex_lock(&vcpu->kvm->arch.config_lock);
> + ret = arm64_check_features(vcpu, rd, val);
> + if (ret) {
> + mutex_unlock(&vcpu->kvm->arch.config_lock);
> + return ret;
> + }
> + if (val != host_val)
> + vcpu->kvm->arch.ctr_el0 = val;
> + else
> + vcpu->kvm->arch.ctr_el0 = 0;
> +
> + mutex_unlock(&vcpu->kvm->arch.config_lock);
> +
> + clidr_el1 = get_sys_reg_desc(SYS_CLIDR_EL1);
> + if (clidr_el1)
> + clidr_el1->reset(vcpu, clidr_el1);
> +
> + return 0;
No check against what can be changed, and in what direction? You seem
to be allowing a guest to migrate from a host with IDC==1 to one where
IDC==0 (same for DIC). How can that work? Same for the cache lines,
which can be larger on the target... How will the guest survive that?
> +}
> +
> static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> {
> @@ -2460,7 +2509,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
> { SYS_DESC(SYS_SMIDR_EL1), undef_access },
> { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> - { SYS_DESC(SYS_CTR_EL0), access_ctr },
> + { SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> + .get_user = get_ctr, .set_user = set_ctr, .val = (CTR_EL0_DIC_MASK |
> + CTR_EL0_IDC_MASK |
> + CTR_EL0_DminLine_MASK |
> + CTR_EL0_IminLine_MASK)},
> { SYS_DESC(SYS_SVCR), undef_access },
>
> { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
> @@ -3623,6 +3676,13 @@ static bool index_to_params(u64 id, struct sys_reg_params *params)
> }
> }
>
> +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding)
> +{
> + struct sys_reg_params params = encoding_to_params(encoding);
> +
> + return find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> +}
> +
> const struct sys_reg_desc *get_reg_by_id(u64 id,
> const struct sys_reg_desc table[],
> unsigned int num)
> @@ -3676,18 +3736,11 @@ FUNCTION_INVARIANT(midr_el1)
> FUNCTION_INVARIANT(revidr_el1)
> FUNCTION_INVARIANT(aidr_el1)
>
> -static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
> -{
> - ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> - return ((struct sys_reg_desc *)r)->val;
> -}
> -
> /* ->val is filled in by kvm_sys_reg_table_init() */
> static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
> { SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 },
> { SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 },
> { SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 },
> - { SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 },
> };
>
> static int get_invariant_sys_reg(u64 id, u64 __user *uaddr)
> @@ -4049,6 +4102,9 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
> vcpu->arch.hcrx_el2 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2);
> }
>
> + if (vcpu->kvm->arch.ctr_el0)
> + vcpu->arch.hcr_el2 |= HCR_TID2;
Why trap CTR_EL0 if the values are the same as the host? I really
dislike the use of the value 0 as a such an indication. Why isn't this
grouped with the traps in vcpu_reset_hcr()?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists