linux-kernel - Re: [PATCH v4 3/6] 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]

Date: Thu, 13 Jun 2024 22:30:15 +0000
From: Oliver Upton <oliver.upton@...ux.dev>
To: Sebastian Ott <sebott@...hat.com>
Cc: linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, Marc Zyngier <maz@...nel.org>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Shaoqin Huang <shahuang@...hat.com>,
	Eric Auger <eric.auger@...hat.com>
Subject: Re: [PATCH v4 3/6] KVM: arm64: add emulation for CTR_EL0 register

On Thu, Jun 13, 2024 at 10:19:56PM +0000, Oliver Upton wrote:
> Hi Sebastian,
> 
> On Mon, Jun 03, 2024 at 03:05:04PM +0200, Sebastian Ott wrote:
> 
> [...]
> 
> > +static int validate_cache_topology(struct kvm_vcpu *vcpu, u64 ctr_el0)
> > +{
> > +	const struct sys_reg_desc *clidr_el1;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	clidr_el1 = get_sys_reg_desc(SYS_CLIDR_EL1);
> > +	if (!clidr_el1)
> > +		return -ENOENT;
> 
> This doesn't actually matter if we agree on dropping the cross-checking,
> but if this lookup fails it is 100% a KVM bug. Returning ENOENT isn't
> exactly right here, since it gives userspace the impression that the
> sysreg index it tried to access does not exist.
> 
> So in the future it'd be good to return EINVAL in places where the
> kernel did something stupid, probably with a warning for good measure.
> 
> > +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > +		   u64 val)
> > +{
> > +	u64 ctr, writable_mask = rd->val;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&vcpu->kvm->arch.config_lock);
> > +	ctr  = vcpu->kvm->arch.ctr_el0;
> > +	if (val == ctr)
> > +		goto out_unlock;
> > +
> > +	ret = -EBUSY;
> > +	if (kvm_vm_has_ran_once(vcpu->kvm))
> > +		goto out_unlock;
> > +
> > +	ret = -EINVAL;
> > +	if ((ctr & ~writable_mask) != (val & ~writable_mask))
> > +		goto out_unlock;
> > +
> > +	if (((ctr & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)) ||
> > +	    ((ctr & CTR_EL0_IDC_MASK) < (val & CTR_EL0_IDC_MASK)) ||
> > +	    ((ctr & CTR_EL0_DminLine_MASK) < (val & CTR_EL0_DminLine_MASK)) ||
> > +	    ((ctr & CTR_EL0_IminLine_MASK) < (val & CTR_EL0_IminLine_MASK))) {
> > +		goto out_unlock;
> 
> I'd prefer if we addressed the issue w/ arm64_check_features() by making
> CTR_EL0 behave like the other registers in the ID space instead of
> open-coding these sorts of checks.
> 
> I believe that can be accomplished by using kvm_read_sanitised_id_reg()
> as the ::reset() function in the descriptor and initializing
> kvm->arch.ctr_el0 in kvm_reset_id_regs().

Durr, I got rid of kvm_reset_id_regs() in commit 44cbe80b7616 ("KVM: arm64:
Reset VM feature ID regs from kvm_reset_sys_regs()"), I should engage
brain before responding.

Adding a check for encoding == CTR_EL0 to is_vm_ftr_id_reg() seems to be
the best way out.

-- 
Thanks,
Oliver