Re: [PATCH v3 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests

[Marc Zyngier <marc dot zyngier at arm dot com>]

On 17/10/17 14:51, Christoffer Dall wrote:
> On Tue, Oct 10, 2017 at 07:38:19PM +0100, Dave Martin wrote:
>> Currently, a guest kernel sees the true CPU feature registers
>> (ID_*_EL1) when it reads them using MRS instructions.  This means
>> that the guest will observe features that are present in the
>> hardware but the host doesn't understand or doesn't provide support
>> for.  A guest may legimitately try to use such a feature as per the
>> architecture, but use of the feature may trap instead of working
>> normally, triggering undef injection into the guest.
>>
>> This is not a problem for the host, but the guest may go wrong when
>> running on newer hardware than the host knows about.
>>
>> This patch hides from guest VMs any AArch64-specific CPU features
>> that the host doesn't support, by exposing to the guest the
>> sanitised versions of the registers computed by the cpufeatures
>> framework, instead of the true hardware registers.  To achieve
>> this, HCR_EL2.TID3 is now set for AArch64 guests, and emulation
>> code is added to KVM to report the sanitised versions of the
>> affected registers in response to MRS and register reads from
>> userspace.
>>
>> The affected registers are removed from invariant_sys_regs[] (since
>> the invariant_sys_regs handling is no longer quite correct for
>> them) and added to sys_reg_desgs[], with appropriate access(),
>> get_user() and set_user() methods.  No runtime vcpu storage is
>> allocated for the registers: instead, they are read on demand from
>> the cpufeatures framework.  This may need modification in the
>> future if there is a need for userspace to customise the features
>> visible to the guest.
>>
>> Attempts by userspace to write the registers are handled similarly
>> to the current invariant_sys_regs handling: writes are permitted,
>> but only if they don't attempt to change the value.  This is
>> sufficient to support VM snapshot/restore from userspace.
>>
>> Because of the additional registers, restoring a VM on an older
>> kernel may not work unless userspace knows how to handle the extra
>> VM registers exposed to the KVM user ABI by this patch.
>>
>> Under the principle of least damage, this patch makes no attempt to
>> handle any of the other registers currently in
>> invariant_sys_regs[], or to emulate registers for AArch32: however,
>> these could be handled in a similar way in future, as necessary.
>>
>> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/sysreg.h |   3 +
>>  arch/arm64/kvm/hyp/switch.c     |   6 +
>>  arch/arm64/kvm/sys_regs.c       | 282 +++++++++++++++++++++++++++++++++-------
>>  3 files changed, 246 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index f707fed..480ecd6 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -149,6 +149,9 @@
>>  #define SYS_ID_AA64DFR0_EL1		sys_reg(3, 0, 0, 5, 0)
>>  #define SYS_ID_AA64DFR1_EL1		sys_reg(3, 0, 0, 5, 1)
>>  
>> +#define SYS_ID_AA64AFR0_EL1		sys_reg(3, 0, 0, 5, 4)
>> +#define SYS_ID_AA64AFR1_EL1		sys_reg(3, 0, 0, 5, 5)
>> +
>>  #define SYS_ID_AA64ISAR0_EL1		sys_reg(3, 0, 0, 6, 0)
>>  #define SYS_ID_AA64ISAR1_EL1		sys_reg(3, 0, 0, 6, 1)
>>  
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 945e79c..35a90b8 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -81,11 +81,17 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>>  	 * it will cause an exception.
>>  	 */
>>  	val = vcpu->arch.hcr_el2;
>> +
>>  	if (!(val & HCR_RW) && system_supports_fpsimd()) {
>>  		write_sysreg(1 << 30, fpexc32_el2);
>>  		isb();
>>  	}
>> +
>> +	if (val & HCR_RW) /* for AArch64 only: */
>> +		val |= HCR_TID3; /* TID3: trap feature register accesses */
>> +
> 
> Since we're setting this for all 64-bit VMs, can we not set this in
> vcpu_reset_hcr instead?
> 
>>  	write_sysreg(val, hcr_el2);
>> +
>>  	/* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>>  	write_sysreg(1 << 15, hstr_el2);
>>  	/*
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2e070d3..b1f7552 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -892,6 +892,137 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu,
>>  	return true;
>>  }
>>  
>> +/* Read a sanitised cpufeature ID register by sys_reg_desc */
>> +static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
>> +{
>> +	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>> +			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
>> +
>> +	return raz ? 0 : read_sanitised_ftr_reg(id);
>> +}
>> +
>> +/* cpufeature ID register access trap handlers */
>> +
>> +static bool __access_id_reg(struct kvm_vcpu *vcpu,
>> +			    struct sys_reg_params *p,
>> +			    const struct sys_reg_desc *r,
>> +			    bool raz)
>> +{
>> +	if (p->is_write)
>> +		return write_to_read_only(vcpu, p, r);
>> +
>> +	p->regval = read_id_reg(r, raz);
>> +	return true;
>> +}
>> +
>> +static bool access_id_reg(struct kvm_vcpu *vcpu,
>> +			  struct sys_reg_params *p,
>> +			  const struct sys_reg_desc *r)
>> +{
>> +	return __access_id_reg(vcpu, p, r, false);
>> +}
>> +
>> +static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
>> +			      struct sys_reg_params *p,
>> +			      const struct sys_reg_desc *r)
>> +{
>> +	return __access_id_reg(vcpu, p, r, true);
>> +}
>> +
>> +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
>> +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
>> +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
>> +
>> +/*
>> + * cpufeature ID register user accessors
>> + *
>> + * For now, these registers are immutable for userspace, so no values
>> + * are stored, and for set_id_reg() we don't allow the effective value
>> + * to be changed.
>> + */
>> +static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
>> +			bool raz)
>> +{
>> +	const u64 id = sys_reg_to_index(rd);
>> +	const u64 val = read_id_reg(rd, raz);
>> +
>> +	return reg_to_user(uaddr, &val, id);
>> +}
>> +
>> +static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
>> +			bool raz)
>> +{
>> +	const u64 id = sys_reg_to_index(rd);
>> +	int err;
>> +	u64 val;
>> +
>> +	err = reg_from_user(&val, uaddr, id);
>> +	if (err)
>> +		return err;
>> +
>> +	/* This is what we mean by invariant: you can't change it. */
>> +	if (val != read_id_reg(rd, raz))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +		      const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> +	return __get_id_reg(rd, uaddr, false);
>> +}
>> +
>> +static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +		      const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> +	return __set_id_reg(rd, uaddr, false);
>> +}
>> +
>> +static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			  const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> +	return __get_id_reg(rd, uaddr, true);
>> +}
>> +
>> +static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			  const struct kvm_one_reg *reg, void __user *uaddr)
>> +{
>> +	return __set_id_reg(rd, uaddr, true);
>> +}
>> +
>> +/* sys_reg_desc initialiser for known cpufeature ID registers */
>> +#define ID_SANITISED(name) {			\
>> +	SYS_DESC(SYS_##name),			\
>> +	.access	= access_id_reg,		\
>> +	.get_user = get_id_reg,			\
>> +	.set_user = set_id_reg,			\
>> +}
>> +
>> +/*
>> + * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
>> + * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
>> + * (1 <= crm < 8, 0 <= Op2 < 8).
>> + */
>> +#define ID_UNALLOCATED(crm, op2) {			\
>> +	Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2),	\
>> +	.access = access_raz_id_reg,			\
>> +	.get_user = get_raz_id_reg,			\
>> +	.set_user = set_raz_id_reg,			\
>> +}
>> +
>> +/*
>> + * sys_reg_desc initialiser for known ID registers that we hide from guests.
>> + * For now, these are exposed just like unallocated ID regs: they appear
>> + * RAZ for the guest.
>> + */
> 
> What is a hidden ID register as opposed to an unallocated one?

A hidden register is one where all the features have been removed (RAZ),
making it similar to an unallocated one.

> Shouldn't one of them presumably cause an undefined exception in the
> guest?

No, that'd be a violation of the architecture. The unallocated ID
registers are required to be RAZ (see table D9-2 in D9.3.1), so that
software can probe for feature without running the risk of getting an UNDEF.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...