| 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: <CAE2XoE_8ihJZQF856w-_F+cgJW7fLWGz7M7Ztoxzw2vE51_m1A@mail.gmail.com>
Date: Sun, 16 Feb 2025 00:16:44 +0800
From: 罗勇刚(Yonggang Luo) <luoyonggang@...il.com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: Sebastian Ott <sebott@...hat.com>, Marc Zyngier <maz@...nel.org>, Joey Gouly <joey.gouly@....com>,
Suzuki K Poulose <suzuki.poulose@....com>, Zenghui Yu <yuzenghui@...wei.com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>, Cornelia Huck <cohuck@...hat.com>,
Eric Auger <eric.auger@...hat.com>, linux-arm-kernel@...ts.infradead.org,
kvmarm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1
On Sat, Feb 15, 2025 at 6:15 PM Oliver Upton <oliver.upton@...ux.dev> wrote:
>
> Hi Sebastian,
>
> On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
> > +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > + u64 val)
> > +{
> > + u32 id = reg_to_encoding(rd);
> > + int ret;
> > +
> > + mutex_lock(&vcpu->kvm->arch.config_lock);
>
> There's quite a few early outs, guard() might be a better fit than
> explicitly dropping the lock.
>
> > + /*
> > + * Since guest access to MIDR_EL1 is not trapped
> > + * set up VPIDR_EL2 to hold the MIDR_EL1 value.
> > + */
> > + if (id == SYS_MIDR_EL1)
> > + write_sysreg(val, vpidr_el2);
>
> This is problematic for a couple reasons:
>
> - If the kernel isn't running at EL2, VPIDR_EL2 is undefined
>
> - VPIDR_EL2 needs to be handled as part of the vCPU context, not
> written to without a running vCPU. What would happen if two vCPUs
> have different MIDR values?
>
> Here's a new diff with some hacks thrown in to handle VPIDR_EL2
> correctly. Very lightly tested :)
Thans, I am also faced this issue, but other than this, I am also
facing a issue, after updating
MIDR_EL1, The CP15 register MIDR for aarch32 not updated.
The instruction is `MRC p15,0,<Rt>,c0,c0,0 ; Read CP15 Main ID Register` from
https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/CP15-registers-for-a-PMSA-implementation/c0--Main-ID-Register--MIDR-
The value of this instruction is not updated
>
> Thanks,
> Oliver
> ---
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cfa024de4e3..3db8c773339e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -373,6 +373,7 @@ struct kvm_arch {
> #define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> u64 id_regs[KVM_ARM_ID_REG_NUM];
>
> + u64 midr_el1;
> u64 ctr_el0;
>
> /* Masks for VNCR-backed and general EL2 sysregs */
> @@ -1469,6 +1470,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
> switch (reg) {
> case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
> return &ka->id_regs[IDREG_IDX(reg)];
> + case SYS_MIDR_EL1:
> + return &ka->midr_el1;
> case SYS_CTR_EL0:
> return &ka->ctr_el0;
> default:
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index 76ff095c6b6e..866411621a39 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -168,9 +168,11 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
> }
>
> static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
> - u64 mpidr)
> + u64 midr, u64 mpidr)
> {
> - write_sysreg(mpidr, vmpidr_el2);
> + write_sysreg(midr, vpidr_el2);
> + write_sysreg(mpidr, vmpidr_el2);
> +
>
> if (has_vhe() ||
> !cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
> diff --git a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> index dba101565de3..a01be1add5ad 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> @@ -28,7 +28,15 @@ void __sysreg_save_state_nvhe(struct kvm_cpu_context *ctxt)
>
> void __sysreg_restore_state_nvhe(struct kvm_cpu_context *ctxt)
> {
> - __sysreg_restore_el1_state(ctxt, ctxt_sys_reg(ctxt, MPIDR_EL1));
> + u64 midr;
> +
> + if (ctxt_is_guest(ctxt))
> + midr = kvm_read_vm_id_reg(kern_hyp_va(ctxt_to_vcpu(ctxt)->kvm),
> + SYS_MIDR_EL1);
> + else
> + midr = read_cpuid_id();
> +
> + __sysreg_restore_el1_state(ctxt, midr, ctxt_sys_reg(ctxt, MPIDR_EL1));
> __sysreg_restore_common_state(ctxt);
> __sysreg_restore_user_state(ctxt);
> __sysreg_restore_el2_return_state(ctxt);
> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index 90b018e06f2c..1d4b9597eb29 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -87,11 +87,12 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
> write_sysreg(__vcpu_sys_reg(vcpu, PAR_EL1), par_el1);
> write_sysreg(__vcpu_sys_reg(vcpu, TPIDR_EL1), tpidr_el1);
>
> - write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1), vmpidr_el2);
> - write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2), SYS_MAIR);
> - write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2), SYS_VBAR);
> - write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2), SYS_CONTEXTIDR);
> - write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2), SYS_AMAIR);
> + write_sysreg(kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1), vpidr_el2);
> + write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1), vmpidr_el2);
> + write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2), SYS_MAIR);
> + write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2), SYS_VBAR);
> + write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2), SYS_CONTEXTIDR);
> + write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2), SYS_AMAIR);
>
> if (vcpu_el2_e2h_is_set(vcpu)) {
> /*
> @@ -191,7 +192,7 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
> {
> struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
> struct kvm_cpu_context *host_ctxt;
> - u64 mpidr;
> + u64 midr, mpidr;
>
> host_ctxt = host_data_ptr(host_ctxt);
> __sysreg_save_user_state(host_ctxt);
> @@ -222,10 +223,9 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
> if (vcpu_has_nv(vcpu)) {
> /*
> * Use the guest hypervisor's VPIDR_EL2 when in a
> - * nested state. The hardware value of MIDR_EL1 gets
> - * restored on put.
> + * nested state.
> */
> - write_sysreg(ctxt_sys_reg(guest_ctxt, VPIDR_EL2), vpidr_el2);
> + midr = ctxt_sys_reg(guest_ctxt, VPIDR_EL2);
>
> /*
> * As we're restoring a nested guest, set the value
> @@ -233,10 +233,11 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
> */
> mpidr = ctxt_sys_reg(guest_ctxt, VMPIDR_EL2);
> } else {
> + midr = kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1);
> mpidr = ctxt_sys_reg(guest_ctxt, MPIDR_EL1);
> }
>
> - __sysreg_restore_el1_state(guest_ctxt, mpidr);
> + __sysreg_restore_el1_state(guest_ctxt, midr, mpidr);
> }
>
> vcpu_set_flag(vcpu, SYSREGS_ON_CPU);
> @@ -271,9 +272,5 @@ void __vcpu_put_switch_sysregs(struct kvm_vcpu *vcpu)
> /* Restore host user state */
> __sysreg_restore_user_state(host_ctxt);
>
> - /* If leaving a nesting guest, restore MIDR_EL1 default view */
> - if (vcpu_has_nv(vcpu))
> - write_sysreg(read_cpuid_id(), vpidr_el2);
> -
> vcpu_clear_flag(vcpu, SYSREGS_ON_CPU);
> }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f6cd1ea7fb55..aa1a0443dc6a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1656,7 +1656,7 @@ static bool is_feature_id_reg(u32 encoding)
> */
> static inline bool is_vm_ftr_id_reg(u32 id)
> {
> - if (id == SYS_CTR_EL0)
> + if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1)
> return true;
>
> return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> @@ -1989,6 +1989,34 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> return 0;
> }
>
> +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> + u64 val)
> +{
> + u32 id = reg_to_encoding(rd);
> +
> + guard(mutex)(&vcpu->kvm->arch.config_lock);
> +
> + /*
> + * Once the VM has started the ID registers are immutable. Reject any
> + * write that does not match the final register value.
> + */
> + if (kvm_vm_has_ran_once(vcpu->kvm)) {
> + if (val != read_id_reg(vcpu, rd))
> + return -EBUSY;
> +
> + return 0;
> + }
> +
> + /*
> + * For non ftr regs do a limited test against the writable mask only.
> + */
> + if ((rd->val & val) != val)
> + return -EINVAL;
> +
> + kvm_set_vm_id_reg(vcpu->kvm, id, val);
> + return 0;
> +}
> +
> static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> u64 val)
> {
> @@ -2483,6 +2511,15 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
> return true;
> }
>
> +#define FUNCTION_RESET(reg) \
> + static u64 reset_##reg(struct kvm_vcpu *v, \
> + const struct sys_reg_desc *r) \
> + { \
> + return read_sysreg(reg); \
> + }
> +
> +FUNCTION_RESET(midr_el1)
> +
>
> /*
> * Architected system registers.
> @@ -2532,6 +2569,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>
> { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
>
> + { ID_DESC(MIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
> + .reset = reset_midr_el1, .val = GENMASK_ULL(31, 0) },
> { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
>
> /*
> @@ -4584,13 +4623,11 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
> return ((struct sys_reg_desc *)r)->val; \
> }
>
> -FUNCTION_INVARIANT(midr_el1)
> FUNCTION_INVARIANT(revidr_el1)
> FUNCTION_INVARIANT(aidr_el1)
>
> /* ->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, reset_midr_el1 },
> { SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 },
> { SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
> };
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Powered by blists - more mailing lists