linux-kernel - Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR

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] [day] [month] [year] [list]

Message-ID: <afac82bb-2d7d-66d0-cdb8-d8e3471a2bed@redhat.com>
Date: Mon, 17 Feb 2025 16:06:16 +0100 (CET)
From: Sebastian Ott <sebott@...hat.com>
To: Oliver Upton <oliver.upton@...ux.dev>
cc: Marc Zyngier <maz@...nel.org>, Joey Gouly <joey.gouly@....com>, 
    Suzuki K Poulose <suzuki.poulose@....com>, 
    Zenghui Yu <yuzenghui@...wei.com>, 
    Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
    Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>, 
    Cornelia Huck <cohuck@...hat.com>, Eric Auger <eric.auger@...hat.com>, 
    linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

Hello Oliver,

On Sat, 15 Feb 2025, Oliver Upton wrote:
> On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
>> +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			      u64 val)
>> +{
>> +	u32 id = reg_to_encoding(rd);
>> +	int ret;
>> +
>> +	mutex_lock(&vcpu->kvm->arch.config_lock);
>
> There's quite a few early outs, guard() might be a better fit than
> explicitly dropping the lock.

Yea, I thought about that too but most of the other functions in that file
use the classic lock primitives. But you're right - it looks cleaner.

>
>> +	/*
>> +	 * Since guest access to MIDR_EL1 is not trapped
>> +	 * set up VPIDR_EL2 to hold the MIDR_EL1 value.
>> +	 */
>> +	if (id == SYS_MIDR_EL1)
>> +		write_sysreg(val, vpidr_el2);
>
> This is problematic for a couple reasons:
>
> - If the kernel isn't running at EL2, VPIDR_EL2 is undefined
>
> - VPIDR_EL2 needs to be handled as part of the vCPU context, not
>   written to without a running vCPU. What would happen if two vCPUs
>   have different MIDR values?

Indeed. Sry, I hadn't thought about that. That makes much more sense now.

> Here's a new diff with some hacks thrown in to handle VPIDR_EL2
> correctly. Very lightly tested :)

Thank you very much! I've integrated that and currently run some tests
with it.

Sebastian