linux-kernel - Re: [PATCH 19/26] x86/tdx: Make pages shared in ioremap()

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

Date:   Thu, 23 Dec 2021 11:45:19 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tom Lendacky <thomas.lendacky@....com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        luto@...nel.org, peterz@...radead.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
        ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
        joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
        pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
        tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 19/26] x86/tdx: Make pages shared in ioremap()

On 12/23/21 9:15 AM, Kirill A. Shutemov wrote:
>>> +pgprot_t pgprot_cc_encrypted(pgprot_t prot)
>>> +{
>>> +	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
>>> +		return __pgprot(__sme_set(pgprot_val(prot)));
>>> +	else if (cc_platform_has(CC_ATTR_GUEST_TDX))
>>> +		return __pgprot(pgprot_val(prot) & ~tdx_shared_mask());
>>> +
>> Hmmm... I believe this breaks SEV guests. __sme_set() uses sme_me_mask which
>> is used for both SME and SEV. With the current checks, an SEV guest will end
>> up never setting an encrypted address through this path. Ditto below on the
>> decrypted path.
> Hm, okay. What if I rewrite code like this:
> 
> 	pgprot_t pgprot_cc_encrypted(pgprot_t prot)
> 	{
> 		if (cc_platform_has(CC_ATTR_GUEST_TDX))
> 			return __pgprot(pgprot_val(prot) & ~tdx_shared_mask());
> 		else
> 			return __pgprot(__sme_set(pgprot_val(prot)));
> 	}
> 
> I believe it should cover all cases, right?

I _think_ that should be fine for now.  But, it does expose that
__sme_set() is weird because it get used on non-SME systems while
tdx_shared_mask() is only used on TDX systems.

Ideally, we'd eventually get to something close to what you had originally:

pgprot_t pgprot_cc_encrypted(pgprot_t prot)
{
	if (cc_platform_has(CC_ATTR_GUEST_TDX))
		return __pgprot(pgprot_val(prot) & ~tdx_shared_mask());
	if (cc_platform_has(CC_ATTR_SME_SOMETHING??))
		return __pgprot(pgprot_val(prot) | sme_me_mask));

	return prot;
}

CC_ATTR_SME_SOMETHING would get set when sme_me_mask is initialized to
something non-zero.  That will keep folks from falling into the same
trap that you did in the long term.

The SEV code wasn't crazy for doing what it did when it was the only
game in town.  But, now that TDX is joining the party, we need to make
sure that SEV isn't special.