linux-kernel - Re: [PATCH v5 2/7] KVM: VMX: Add proper cache tracking for PKRS

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: <YYlnSdw+3dY/lJ25@google.com>
Date:   Mon, 8 Nov 2021 18:07:05 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Chenyi Qiang <chenyi.qiang@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Xiaoyao Li <xiaoyao.li@...el.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/7] KVM: VMX: Add proper cache tracking for PKRS

On Mon, Nov 08, 2021, Sean Christopherson wrote:
> On Wed, Aug 11, 2021, Chenyi Qiang wrote:
> > +			vcpu->arch.pkrs = vmcs_read64(GUEST_IA32_PKRS);
> 
> Hrm.  I agree that it's extremely unlikely that IA32_PKRS will ever allow software
> to set bits 63:32, but at the same time there's no real advantage to KVM it as a u32,
> e.g. the extra 4 bytes per vCPU is a non-issue, and could be avoided by shuffling
> kvm_vcpu_arch to get a more efficient layout.  On the flip side, using a 32 means
> code like this _looks_ buggy because it's silently dropping bits 63:32, and if the
> architecture ever does get updated, we'll have to modify a bunch of KVM code.
> 
> TL;DR: I vote to track PRKS as a u64 even though the kernel tracks it as a u32.

Rats, I forgot that the MMU code for PKRU is going to be reused for PKRS.  I withdraw
my vote :-)

Maybe have this code WARN on bits 63:32 being set in the VMCS field?  E.g. to
detect if hardware ever changes and KVM fails to update this path, and to document
that KVM intentionally drops those bits.

	case VCPU_EXREG_PKRS: {
		u64 ia32_pkrs;

		ia32_pkrs = vmcs_read64(GUEST_IA32_PKRS);
		WARN_ON_ONCE(ia32_pkrs >> 32);
		vcpu->arch.pkrs = (u32)ia32_pkrs;
	}