| Message ID | 1669951831-4180-5-git-send-email-mikelley@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PCI pass-thru support to Hyper-V Confidential VMs | expand |
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. :-)
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
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)
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.
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.
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 --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");
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(-)