diff mbox series

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

Message ID 1669951831-4180-5-git-send-email-mikelley@microsoft.com
State New
Headers
Series Add PCI pass-thru support to Hyper-V Confidential VMs |

Commit Message

Michael Kelley (LINUX) Dec. 2, 2022, 3:30 a.m. UTC 
  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.

Fixes: e9d1d2bb75b2 ("treewide: Replace the use of mem_encrypt_active() with cc_platform_has()")
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/mm/mem_encrypt_amd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Borislav Petkov Dec. 29, 2022, 12:17 p.m. UTC | #1 
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."

> 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. :-)
Michael Kelley (LINUX) Dec. 29, 2022, 4:25 p.m. UTC | #2 
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
Bjorn Helgaas Dec. 29, 2022, 4:54 p.m. UTC | #3 
On Thu, Dec 29, 2022 at 01:17:48PM +0100, Borislav Petkov wrote:
> 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.

s/ection/section/

(In case you copy/paste this text without noticing the typo)
Borislav Petkov Dec. 29, 2022, 5:12 p.m. UTC | #4 
On Thu, Dec 29, 2022 at 10:54:31AM -0600, Bjorn Helgaas wrote:
> > "sme_postprocess_startup() decrypts the bss_decrypted ection when me_mask
> > sme_is non-zero.
> 
> s/ection/section/
> 
> (In case you copy/paste this text without noticing the typo)

Thanks.

My par-agraph reformating helper does mangle words like that sometimes.
The correct sentence should be:

"sme_postprocess_startup() decrypts the bss_decrypted section when
sme_me_mask is non-zero."

/me makes a mental note to switch to the vim builtin instead.
Borislav Petkov Jan. 9, 2023, 7:10 p.m. UTC | #5 
On Thu, Dec 29, 2022 at 04:25:16PM +0000, Michael Kelley (LINUX) wrote:
> 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.

Right, just talked to Tom offlist.

A Fixes tag triggers a lot of backporting activity and if it is not really
needed, then let's leave it out.

If distros decide to pick up vTOM support, then they'll pick up the whole set
anyway.

And if we decide we really need it backported for whatever reason, we will
simply send it into stable and the same backporting activity will be triggered
then. But then we'd at least have a concrete reason for it.

Makes sense?

Thx.
Michael Kelley (LINUX) Jan. 9, 2023, 7:14 p.m. UTC | #6 
From: Borislav Petkov <bp@alien8.de> Sent: Monday, January 9, 2023 11:11 AM
> 
> On Thu, Dec 29, 2022 at 04:25:16PM +0000, Michael Kelley (LINUX) wrote:
> > 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.
> 
> Right, just talked to Tom offlist.
> 
> A Fixes tag triggers a lot of backporting activity and if it is not really
> needed, then let's leave it out.
> 
> If distros decide to pick up vTOM support, then they'll pick up the whole set
> anyway.
> 
> And if we decide we really need it backported for whatever reason, we will
> simply send it into stable and the same backporting activity will be triggered
> then. But then we'd at least have a concrete reason for it.
> 
> Makes sense?
> 

Yep, that matches my thinking.  I've avoided marking something for stable unless
it fixes something that is actually broken.

Michael
diff mbox series

Patch

diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index 9c4d8db..e0b51c0 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -513,10 +513,14 @@  void __init mem_encrypt_free_decrypted_mem(void)
 	npages = (vaddr_end - vaddr) >> PAGE_SHIFT;
 
 	/*
-	 * The unused memory range was mapped decrypted, change the encryption
-	 * attribute from decrypted to encrypted before freeing it.
+	 * If the unused memory range was mapped decrypted, change the encryption
+	 * attribute from decrypted to encrypted before freeing it. Base the
+	 * re-encryption on the same condition used for the decryption in
+	 * sme_postprocess_startup(). Higher level abstractions, such as
+	 * CC_ATTR_MEM_ENCRYPT, aren't necessarily equivalent in a Hyper-V VM
+	 * using vTOM, where sme_me_mask is always zero.
 	 */
-	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
+	if (sme_me_mask) {
 		r = set_memory_encrypted(vaddr, npages);
 		if (r) {
 			pr_warn("failed to free unused decrypted pages\n");