linux-kernel - Re: [PATCH v5 6/7] KVM: arm64: Mask FEAT

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Y7sRdy13b4NoN/lH@thinky-boi>
Date:   Sun, 8 Jan 2023 10:54:47 -0800
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Akihiko Odaki <akihiko.odaki@...nix.com>
Cc:     Mark Brown <broonie@...nel.org>, Marc Zyngier <maz@...nel.org>,
        linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
        kvmarm@...ts.cs.columbia.edu, linux-arm-kernel@...ts.infradead.org,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        James Morse <james.morse@....com>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        asahi@...ts.linux.dev, Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Sven Peter <sven@...npeter.dev>,
        Hector Martin <marcan@...can.st>
Subject: Re: [PATCH v5 6/7] KVM: arm64: Mask FEAT_CCIDX

Akihiko,

On Sat, Jan 07, 2023 at 06:53:28PM +0900, Akihiko Odaki wrote:
> On 2023/01/06 7:22, Oliver Upton wrote:
> > On Fri, Dec 30, 2022 at 06:54:51PM +0900, Akihiko Odaki wrote:
> > > The CCSIDR access handler masks the associativity bits according to the
> > > bit layout for processors without FEAT_CCIDX. KVM also assumes CCSIDR is
> > > 32-bit where it will be 64-bit if FEAT_CCIDX is enabled. Mask FEAT_CCIDX
> > > so that these assumptions hold.
> > > 
> > > Suggested-by: Marc Zyngier <maz@...nel.org>
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
> > 
> > FYI, I'm an idiot and replied to v4 of this patch... Forwarding comments
> > below:
> > 
> > > ---
> > >   arch/arm64/kvm/sys_regs.c | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index f4a7c5abcbca..aeabf1f3370b 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1124,6 +1124,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> > >   						      ID_DFR0_PERFMON_SHIFT,
> > >   						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> > >   		break;
> > > +	case SYS_ID_AA64MMFR2_EL1:
> > > +		val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
> > > +		break;
> > > +	case SYS_ID_MMFR4_EL1:
> > > +		val &= ~ARM64_FEATURE_MASK(ID_MMFR4_CCIDX);
> > > +		break;
> > 
> > Not that it is necessarily worth addressing, but I wanted to point
> > something out.
> > 
> > This change breaks migration from older kernels on implementations w/
> > FEAT_CCIDX. There is most likely exactly 0 of those in the wild, but
> > we need to be careful changing user-visible stuff like this.
> > 
> > --
> > Thanks,
> > Oliver
> 
> I also don't think whether FEAT_CCIDX is visible matters for any guest
> because the line size a guest would care is held in the same bits whether
> FEAT_CCIDX is implemented. But if it concerns you I can write a bit more
> code so that it won't mask CCIDX bit if it's set from the userspace.

The particular issue I'm alluding to is the fact that KVM treats the ID
registers as invariant. Userspace will save/restore the ID registers for
the VM as part of a migration. Existing kernels advertize FEAT_CCIDX,
whereas kernels with this patch will not. KVM will return an error
(EINVAL) when ID_AA64MMFR2_EL1 is written by userspace.

We've worked around this issue in the past by implementing set_user
calls for ID registers that have changed to tolerate the 'old' value
that KVM advertized.

In any case, it may not be worth addressing given that there are no
implementations in the wild with FEAT_CCIDX. But I wanted to bring it up
on the list for the sake of posterity and also allow anyone to scream
who might be adversely affected by the change.

--
Thanks,
Oliver