linux-kernel - Re: [PATCH v6 5/7] KVM: MMU: Add support for PKS emulation

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:   Wed, 30 Mar 2022 21:27:56 +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 v6 5/7] KVM: MMU: Add support for PKS emulation

On Mon, Feb 21, 2022, Chenyi Qiang wrote:
> @@ -277,14 +278,18 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
>  	if (unlikely(mmu->pkr_mask)) {
>  		u32 pkr_bits, offset;
> +		u32 pkr;
>  
>  		/*
> -		* PKRU defines 32 bits, there are 16 domains and 2
> -		* attribute bits per domain in pkru.  pte_pkey is the
> -		* index of the protection domain, so pte_pkey * 2 is
> -		* is the index of the first bit for the domain.
> +		* PKRU and PKRS both define 32 bits. There are 16 domains
> +		* and 2 attribute bits per domain in them. pte_key is the
> +		* index of the protection domain, so pte_pkey * 2 is the
> +		* index of the first bit for the domain. The use of PKRU
> +		* versus PKRS is selected by the address type, as determined
> +		* by the U/S bit in the paging-structure entries.
>  		*/
> -		pkr_bits = (vcpu->arch.pkru >> (pte_pkey * 2)) & 3;
> +		pkr = pte_access & PT_USER_MASK ? vcpu->arch.pkru : kvm_read_pkrs(vcpu);

Blindly reading PKRU/PKRS is wrong.  I think this magic insanity will be functionally
correct due to update_pkr_bitmask() clearing the appropriate bits in pkr_mask based
on CR4.PK*, but the read should never happen.  PKRU is benign, but I believe reading
PKRS will result in VMREAD to an invalid field if PKRU is supported and enabled, but
PKRS is not supported.

I belive the easiest solution is:

		if (pte_access & PT_USER_MASK)
			pkr = is_cr4_pke(mmu) ? vcpu->arch.pkru : 0;
		else
			pkr = is_cr4_pks(mmu) ? kvm_read_pkrs(vcpu) : 0;

The is_cr4_pk*() helpers are restricted to mmu.c, but this presents a good
opportunity to extra the PKR stuff to a separate, non-inline helper (as a prep
patch).  E.g.


	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
	if (unlikely(mmu->pkr_mask))
		u32 pkr_bits = kvm_mmu_pkr_bits(vcpu, mmu, pte_access, pte_pkey);

		errcode |= -pkr_bits & PFERR_PK_MASK;
		fault |= (pkr_bits != 0);
	}

	return -(u32)fault & errcode;

permission_fault() is inline because it's heavily used for shadow paging, but
when using TDP, it's far less performance critical.  PKR is TDP-only, so moving
it out-of-line should be totally ok (this is also why this patch is "unlikely").