Re: [PATCH v12 07/11] KVM: arm64: Use arm64_ftr_bits to sanitise ID register writes

[Marc Zyngier <maz@kernel.org>]

Hi Oliver,

On Fri, 09 Jun 2023 20:00:50 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> From: Jing Zhang <jingzhangos@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@google.com>
> [Oliver: split off from monster patch, rewrote commit description,
>  reworked RAZ handling]
> Signed-off-by: Oliver Upton <oliver.upton@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.