linux-kernel - Re: [PATCH 3/4] KVM: arm64: add emulation for CTR

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7cc16dc9-6eef-59f9-d019-8b5dea6a4254@redhat.com>
Date: Sat, 13 Apr 2024 15:50:42 +0200 (CEST)
From: Sebastian Ott <sebott@...hat.com>
To: Marc Zyngier <maz@...nel.org>
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 Sat, 13 Apr 2024, Marc Zyngier wrote:

> 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...

Yea, that's why I've asked in the cover letter if userspace would be
ok with that. I thought that this is what you suggested in your reply
to the RFC. 
(https://lore.kernel.org/linux-arm-kernel/20240318111636.10613-5-sebott@redhat.com/T/#m0aea5b744774f123bd9a4a4b7be6878f6c737f63)

But I guess I've got that wrong.

Do we have other means to handle the dependencies between registers?
Allow inconsistent values and do a sanity check before the first
vcpu_run()?

>>
>> 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?

The idea was that kvm->arch.ctr_el0 == 0 means we use the host value and
don't set up a trap.

>> +}
>> +
>> +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?

Shouldn't this all be handled by arm64_check_features() using the safe
value definitions from ftr_ctr? (I'll double check that..)

>> @@ -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?

For values same as host vcpu->kvm->arch.ctr_el0 would be zero and
reg access would not be trapped.

> I really dislike the use of the value 0 as a such an indication.

OK.

> Why isn't this grouped with the traps in vcpu_reset_hcr()?

I was under the impression that vcpu_reset_hcr() is called too early
to decide if we need to set up a trap or not (but lemme double check
that).

Thanks,
Sebastian