[PATCH v12 07/11] KVM: arm64: Use arm64_ftr_bits to sanitise ID register writes
Marc Zyngier
maz at kernel.org
Thu Jun 15 05:38:34 PDT 2023
Hi Oliver,
On Fri, 09 Jun 2023 20:00:50 +0100,
Oliver Upton <oliver.upton at linux.dev> wrote:
>
> From: Jing Zhang <jingzhangos at google.com>
>
> Rather than reinventing the wheel in KVM to do ID register sanitisation
> we can rely on the work already done in the core kernel. Implement a
> generalized sanitisation of ID registers based on the combination of the
> arm64_ftr_bits definitions from the core kernel and (optionally) a set
> of KVM-specific overrides.
>
> This all amounts to absolutely nothing for now, but will be used in
> subsequent changes to realize user-configurable ID registers.
>
> Signed-off-by: Jing Zhang <jingzhangos at google.com>
> [Oliver: split off from monster patch, rewrote commit description,
> reworked RAZ handling]
> Signed-off-by: Oliver Upton <oliver.upton at linux.dev>
> ---
> arch/arm64/include/asm/cpufeature.h | 1 +
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kvm/sys_regs.c | 113 +++++++++++++++++++++++++++-
> 3 files changed, 111 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 6bf013fb110d..dc769c2eb7a4 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -915,6 +915,7 @@ static inline unsigned int get_vmid_bits(u64 mmfr1)
> return 8;
> }
>
> +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, s64 cur);
> struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id);
>
> extern struct arm64_ftr_override id_aa64mmfr1_override;
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 7d7128c65161..3317a7b6deac 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -798,7 +798,7 @@ static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
> return reg;
> }
>
> -static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
> +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
> s64 cur)
> {
> s64 ret = 0;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 3015c860deca..0fbdb6ef68e4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1201,6 +1201,91 @@ static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
> return 0;
> }
>
> +static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
> + s64 new, s64 cur)
> +{
> + struct arm64_ftr_bits kvm_ftr = *ftrp;
> +
> + /* Some features have different safe value type in KVM than host features */
> + switch (id) {
> + case SYS_ID_AA64DFR0_EL1:
> + if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
> + kvm_ftr.type = FTR_LOWER_SAFE;
> + break;
> + case SYS_ID_DFR0_EL1:
> + if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
> + kvm_ftr.type = FTR_LOWER_SAFE;
> + break;
> + }
> +
> + return arm64_ftr_safe_value(&kvm_ftr, new, cur);
> +}
> +
> +/**
> + * arm64_check_features() - Check if a feature register value constitutes
> + * a subset of features indicated by the idreg's KVM sanitised limit.
> + *
> + * This function will check if each feature field of @val is the "safe" value
> + * against idreg's KVM sanitised limit return from reset() callback.
> + * If a field value in @val is the same as the one in limit, it is always
> + * considered the safe value regardless For register fields that are not in
> + * writable, only the value in limit is considered the safe value.
> + *
> + * Return: 0 if all the fields are safe. Otherwise, return negative errno.
> + */
> +static int arm64_check_features(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd,
> + u64 val)
> +{
> + const struct arm64_ftr_reg *ftr_reg;
> + const struct arm64_ftr_bits *ftrp = NULL;
> + u32 id = reg_to_encoding(rd);
> + u64 writable_mask = rd->val;
> + u64 limit = rd->reset(vcpu, rd);
> + u64 mask = 0;
> +
> + /*
> + * Hidden and unallocated ID registers may not have a corresponding
> + * struct arm64_ftr_reg. Of course, if the register is RAZ we know the
> + * only safe value is 0.
> + */
> + if (sysreg_visible_as_raz(vcpu, rd))
> + return val ? -E2BIG : 0;
This -E2BIG is certainly reflecting the error here. However...
> +
> + ftr_reg = get_arm64_ftr_reg(id);
> + if (!ftr_reg)
> + return -EINVAL;
> +
> + ftrp = ftr_reg->ftr_bits;
> +
> + for (; ftrp && ftrp->width; ftrp++) {
> + s64 f_val, f_lim, safe_val;
> + u64 ftr_mask;
> +
> + ftr_mask = arm64_ftr_mask(ftrp);
> + if ((ftr_mask & writable_mask) != ftr_mask)
> + continue;
> +
> + f_val = arm64_ftr_value(ftrp, val);
> + f_lim = arm64_ftr_value(ftrp, limit);
> + mask |= ftr_mask;
> +
> + if (f_val == f_lim)
> + safe_val = f_val;
> + else
> + safe_val = kvm_arm64_ftr_safe_value(id, ftrp, f_val, f_lim);
> +
> + if (safe_val != f_val)
> + return -E2BIG;
> + }
> +
> + /* For fields that are not writable, values in limit are the safe values. */
> + if ((val & ~mask) != (limit & ~mask))
> + return -E2BIG;
> +
> + return 0;
> +}
> +
> static u8 perfmon_to_pmuver(u8 perfmon)
> {
> switch (perfmon) {
> @@ -1528,11 +1613,31 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> u64 val)
> {
> - /* This is what we mean by invariant: you can't change it. */
> - if (val != read_id_reg(vcpu, rd))
> - return -EINVAL;
> + u32 id = reg_to_encoding(rd);
> + int ret;
>
> - return 0;
> + mutex_lock(&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))
> + ret = -EBUSY;
> + else
> + ret = 0;
> +
> + mutex_unlock(&vcpu->kvm->arch.config_lock);
> + return ret;
> + }
> +
> + ret = arm64_check_features(vcpu, rd, val);
> + if (!ret)
> + IDREG(vcpu->kvm, id) = val;
> +
> + mutex_unlock(&vcpu->kvm->arch.config_lock);
> + return ret;
... we now end-up with a *new* error code that userspace was never
able to see so far.
This may not be a big deal, but I'd rather err on the side of caution
by keeping the current, slightly less precise error code.
Thoughts?
M.
--
Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list