RE: [Patch v4 04/13] x86/mm: Handle decryption/re-encryption of bss_decrypted consistently

From: "Michael Kelley (LINUX)" <mikelley@microsoft.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "hpa@zytor.com" <hpa@zytor.com>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	Dexuan Cui <decui@microsoft.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"lpieralisi@kernel.org" <lpieralisi@kernel.org>,
	"robh@kernel.org" <robh@kernel.org>,
	"kw@linux.com" <kw@linux.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"hch@infradead.org" <hch@infradead.org>,
	"m.szyprowski@samsung.com" <m.szyprowski@samsung.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"thomas.lendacky@amd.com" <thomas.lendacky@amd.com>,
	"brijesh.singh@amd.com" <brijesh.singh@amd.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"isaku.yamahata@intel.com" <isaku.yamahata@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"jane.chu@oracle.com" <jane.chu@oracle.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"tony.luck@intel.com" <tony.luck@intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>
Subject: RE: [Patch v4 04/13] x86/mm: Handle decryption/re-encryption of bss_decrypted consistently
Date: Thu, 29 Dec 2022 16:25:16 +0000	[thread overview]
Message-ID: <BYAPR21MB16884038F7EE406322181C58D7F39@BYAPR21MB1688.namprd21.prod.outlook.com> (raw)
In-Reply-To: <Y62FbJ1rZ6TVUgml@zn.tnic>

From: Borislav Petkov <bp@alien8.de> Sent: Thursday, December 29, 2022 4:18 AM
> 
> On Thu, Dec 01, 2022 at 07:30:22PM -0800, Michael Kelley wrote:
> > Current code in sme_postprocess_startup() decrypts the bss_decrypted
> > section when sme_me_mask is non-zero.  But code in
> > mem_encrypt_free_decrypted_mem() re-encrypts the unused portion based
> > on CC_ATTR_MEM_ENCRYPT.  In a Hyper-V guest VM using vTOM, these
> > conditions are not equivalent as sme_me_mask is always zero when
> > using vTOM.  Consequently, mem_encrypt_free_decrypted_mem() attempts
> > to re-encrypt memory that was never decrypted.
> >
> > Fix this in mem_encrypt_free_decrypted_mem() by conditioning the
> > re-encryption on the same test for non-zero sme_me_mask.  Hyper-V
> > guests using vTOM don't need the bss_decrypted section to be
> > decrypted, so skipping the decryption/re-encryption doesn't cause
> > a problem.
> 
> Lemme simplify the formulations a bit:
> 
> "sme_postprocess_startup() decrypts the bss_decrypted ection when me_mask
> sme_is non-zero.
> 
> mem_encrypt_free_decrypted_mem() re-encrypts the unused portion based on
> CC_ATTR_MEM_ENCRYPT.
> 
> In a Hyper-V guest VM using vTOM, these conditions are not equivalent
> as sme_me_mask is always zero when using vTOM. Consequently,
> mem_encrypt_free_decrypted_mem() attempts to re-encrypt memory that was
> never decrypted.
> 
> So check sme_me_mask in mem_encrypt_free_decrypted_mem() too.
> 
> Hyper-V guests using vTOM don't need the bss_decrypted section to be
> decrypted, so skipping the decryption/re-encryption doesn't cause a
> problem."

Work for me.  I'll pick up the new wording in v5.

> 
> > Fixes: e9d1d2bb75b2 ("treewide: Replace the use of mem_encrypt_active() with
> cc_platform_has()")
> 
> So when you say Fixes - this is an issue only for vTOM-using VMs and
> before yours, there were none. And yours needs more enablement than just
> this patch.
> 
> So does this one really need to be backported to stable@?
> 
> I'm asking because there's AI which will pick it up based on this Fixes
> tag up but that AI is still not that smart to replace us all. :-)
> 

I'm ambivalent on the backport to stable.  One might argue that older
kernel versions are conceptually wrong in using different conditions for
the decryption and re-encryption.  But as you said, they aren't broken
from a practical standpoint because sme_me_mask and
CC_ATTR_MEM_ENCRYPT are equivalent prior to my patch set.  However,
the email thread with Sathyanarayanan Kuppuswamy, Tom Lendacky,
and Dexuan Cui concluded that a Fixes: tag is appropriate.   See
https://lore.kernel.org/lkml/fbf2cdcc-4ff7-b466-a6af-7a147f3bc94d@amd.com/
and
https://lore.kernel.org/lkml/BYAPR21MB1688A31ED795ED1B5ACB6D26D7099@BYAPR21MB1688.namprd21.prod.outlook.com/

Michael