* [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail [not found] <20230525225847.28592-1-kirill.shutemov@linux.intel.com> @ 2023-05-25 22:58 ` Kirill A. Shutemov 2023-05-25 23:28 ` Sathyanarayanan Kuppuswamy 2023-05-26 2:17 ` Huang, Kai 2023-05-25 22:58 ` [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() Kirill A. Shutemov 1 sibling, 2 replies; 7+ messages in thread From: Kirill A. Shutemov @ 2023-05-25 22:58 UTC (permalink / raw) To: dave.hansen, tglx, mingo, bp Cc: decui, rick.p.edgecombe, sathyanarayanan.kuppuswamy, seanjc, thomas.lendacky, x86, linux-kernel, Kirill A. Shutemov, stable TDX code is going to provide guest.enc_status_change_prepare() that is able to fail. Add a way to return an error from the callback. While there, fix enc_status_change_finish_noop(). It is defined as always-fail now which doesn't make sense for noop. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: stable@vger.kernel.org --- arch/x86/include/asm/x86_init.h | 2 +- arch/x86/kernel/x86_init.c | 4 ++-- arch/x86/mm/mem_encrypt_amd.c | 4 +++- arch/x86/mm/pat/set_memory.c | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h index 88085f369ff6..1ca9701917c5 100644 --- a/arch/x86/include/asm/x86_init.h +++ b/arch/x86/include/asm/x86_init.h @@ -150,7 +150,7 @@ struct x86_init_acpi { * @enc_cache_flush_required Returns true if a cache flush is needed before changing page encryption status */ struct x86_guest { - void (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); + bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); bool (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc); bool (*enc_tlb_flush_required)(bool enc); bool (*enc_cache_flush_required)(void); diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c index d82f4fa2f1bf..64664311ac2b 100644 --- a/arch/x86/kernel/x86_init.c +++ b/arch/x86/kernel/x86_init.c @@ -130,8 +130,8 @@ struct x86_cpuinit_ops x86_cpuinit = { static void default_nmi_init(void) { }; -static void enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { } -static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return false; } +static bool enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return true; } +static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return true; } static bool enc_tlb_flush_required_noop(bool enc) { return false; } static bool enc_cache_flush_required_noop(void) { return false; } static bool is_private_mmio_noop(u64 addr) {return false; } diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c index e0b51c09109f..4f95c449a406 100644 --- a/arch/x86/mm/mem_encrypt_amd.c +++ b/arch/x86/mm/mem_encrypt_amd.c @@ -319,7 +319,7 @@ static void enc_dec_hypercall(unsigned long vaddr, int npages, bool enc) #endif } -static void amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc) +static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc) { /* * To maintain the security guarantees of SEV-SNP guests, make sure @@ -327,6 +327,8 @@ static void amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool */ if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && !enc) snp_set_memory_shared(vaddr, npages); + + return true; } /* Return true unconditionally: return value doesn't matter for the SEV side */ diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 7159cf787613..b8f48ebe753c 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -2151,7 +2151,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc) cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required()); /* Notify hypervisor that we are about to set/clr encryption attribute. */ - x86_platform.guest.enc_status_change_prepare(addr, numpages, enc); + if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc)) + return -EIO; ret = __change_page_attr_set_clr(&cpa, 1); -- 2.39.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail 2023-05-25 22:58 ` [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail Kirill A. Shutemov @ 2023-05-25 23:28 ` Sathyanarayanan Kuppuswamy 2023-05-26 2:17 ` Huang, Kai 1 sibling, 0 replies; 7+ messages in thread From: Sathyanarayanan Kuppuswamy @ 2023-05-25 23:28 UTC (permalink / raw) To: Kirill A. Shutemov, dave.hansen, tglx, mingo, bp Cc: decui, rick.p.edgecombe, seanjc, thomas.lendacky, x86, linux-kernel, stable Hi, On 5/25/23 3:58 PM, Kirill A. Shutemov wrote: > TDX code is going to provide guest.enc_status_change_prepare() that is > able to fail. You can add some info about why you need this? > > Add a way to return an error from the callback. > > While there, fix enc_status_change_finish_noop(). It is defined as > always-fail now which doesn't make sense for noop. IMO, since the above change is an independent fix, I think it is better to split this into a separate patch. Other than above suggestions, the rest of the changes looks fine. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: stable@vger.kernel.org > --- > arch/x86/include/asm/x86_init.h | 2 +- > arch/x86/kernel/x86_init.c | 4 ++-- > arch/x86/mm/mem_encrypt_amd.c | 4 +++- > arch/x86/mm/pat/set_memory.c | 3 ++- > 4 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > index 88085f369ff6..1ca9701917c5 100644 > --- a/arch/x86/include/asm/x86_init.h > +++ b/arch/x86/include/asm/x86_init.h > @@ -150,7 +150,7 @@ struct x86_init_acpi { > * @enc_cache_flush_required Returns true if a cache flush is needed before changing page encryption status > */ > struct x86_guest { > - void (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > + bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > bool (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc); > bool (*enc_tlb_flush_required)(bool enc); > bool (*enc_cache_flush_required)(void); > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c > index d82f4fa2f1bf..64664311ac2b 100644 > --- a/arch/x86/kernel/x86_init.c > +++ b/arch/x86/kernel/x86_init.c > @@ -130,8 +130,8 @@ struct x86_cpuinit_ops x86_cpuinit = { > > static void default_nmi_init(void) { }; > > -static void enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { } > -static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return false; } > +static bool enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return true; } > +static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return true; } > static bool enc_tlb_flush_required_noop(bool enc) { return false; } > static bool enc_cache_flush_required_noop(void) { return false; } > static bool is_private_mmio_noop(u64 addr) {return false; } > diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c > index e0b51c09109f..4f95c449a406 100644 > --- a/arch/x86/mm/mem_encrypt_amd.c > +++ b/arch/x86/mm/mem_encrypt_amd.c > @@ -319,7 +319,7 @@ static void enc_dec_hypercall(unsigned long vaddr, int npages, bool enc) > #endif > } > > -static void amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc) > +static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc) > { > /* > * To maintain the security guarantees of SEV-SNP guests, make sure > @@ -327,6 +327,8 @@ static void amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool > */ > if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && !enc) > snp_set_memory_shared(vaddr, npages); > + > + return true; > } > > /* Return true unconditionally: return value doesn't matter for the SEV side */ > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > index 7159cf787613..b8f48ebe753c 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -2151,7 +2151,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc) > cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required()); > > /* Notify hypervisor that we are about to set/clr encryption attribute. */ > - x86_platform.guest.enc_status_change_prepare(addr, numpages, enc); > + if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc)) > + return -EIO; > > ret = __change_page_attr_set_clr(&cpa, 1); > -- Sathyanarayanan Kuppuswamy Linux Kernel Developer ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail 2023-05-25 22:58 ` [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail Kirill A. Shutemov 2023-05-25 23:28 ` Sathyanarayanan Kuppuswamy @ 2023-05-26 2:17 ` Huang, Kai 2023-05-26 11:49 ` kirill.shutemov 1 sibling, 1 reply; 7+ messages in thread From: Huang, Kai @ 2023-05-26 2:17 UTC (permalink / raw) To: kirill.shutemov@linux.intel.com, tglx@linutronix.de, mingo@redhat.com, Hansen, Dave, bp@alien8.de Cc: Cui, Dexuan, Christopherson,, Sean, x86@kernel.org, Edgecombe, Rick P, sathyanarayanan.kuppuswamy@linux.intel.com, linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, stable@vger.kernel.org On Fri, 2023-05-26 at 01:58 +0300, Kirill A. Shutemov wrote: > TDX code is going to provide guest.enc_status_change_prepare() that is > able to fail. > > Add a way to return an error from the callback. > > While there, fix enc_status_change_finish_noop(). It is defined as > always-fail now which doesn't make sense for noop. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: stable@vger.kernel.org > --- > arch/x86/include/asm/x86_init.h | 2 +- > arch/x86/kernel/x86_init.c | 4 ++-- > arch/x86/mm/mem_encrypt_amd.c | 4 +++- > arch/x86/mm/pat/set_memory.c | 3 ++- > 4 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > index 88085f369ff6..1ca9701917c5 100644 > --- a/arch/x86/include/asm/x86_init.h > +++ b/arch/x86/include/asm/x86_init.h > @@ -150,7 +150,7 @@ struct x86_init_acpi { > * @enc_cache_flush_required Returns true if a cache flush is needed before changing page encryption status > */ > struct x86_guest { > - void (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > + bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > [...] > @@ -2151,7 +2151,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc) > cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required()); > > /* Notify hypervisor that we are about to set/clr encryption attribute. */ > - x86_platform.guest.enc_status_change_prepare(addr, numpages, enc); > + if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc)) > + return -EIO; The name "enc_status_change_prepare()" sounds like an action, but not some true/false condition check. I think it's more reasonable to make it return 'int', and returning 0 means successful? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail 2023-05-26 2:17 ` Huang, Kai @ 2023-05-26 11:49 ` kirill.shutemov 0 siblings, 0 replies; 7+ messages in thread From: kirill.shutemov @ 2023-05-26 11:49 UTC (permalink / raw) To: Huang, Kai Cc: tglx@linutronix.de, mingo@redhat.com, Hansen, Dave, bp@alien8.de, Cui, Dexuan, Christopherson,, Sean, x86@kernel.org, Edgecombe, Rick P, sathyanarayanan.kuppuswamy@linux.intel.com, linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, stable@vger.kernel.org On Fri, May 26, 2023 at 02:17:12AM +0000, Huang, Kai wrote: > On Fri, 2023-05-26 at 01:58 +0300, Kirill A. Shutemov wrote: > > TDX code is going to provide guest.enc_status_change_prepare() that is > > able to fail. > > > > Add a way to return an error from the callback. > > > > While there, fix enc_status_change_finish_noop(). It is defined as > > always-fail now which doesn't make sense for noop. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Cc: stable@vger.kernel.org > > --- > > arch/x86/include/asm/x86_init.h | 2 +- > > arch/x86/kernel/x86_init.c | 4 ++-- > > arch/x86/mm/mem_encrypt_amd.c | 4 +++- > > arch/x86/mm/pat/set_memory.c | 3 ++- > > 4 files changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > > index 88085f369ff6..1ca9701917c5 100644 > > --- a/arch/x86/include/asm/x86_init.h > > +++ b/arch/x86/include/asm/x86_init.h > > @@ -150,7 +150,7 @@ struct x86_init_acpi { > > * @enc_cache_flush_required Returns true if a cache flush is needed before changing page encryption status > > */ > > struct x86_guest { > > - void (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > > + bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc); > > > > [...] > > > @@ -2151,7 +2151,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc) > > cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required()); > > > > /* Notify hypervisor that we are about to set/clr encryption attribute. */ > > - x86_platform.guest.enc_status_change_prepare(addr, numpages, enc); > > + if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc)) > > + return -EIO; > > The name "enc_status_change_prepare()" sounds like an action, but not some > true/false condition check. I think it's more reasonable to make it return > 'int', and returning 0 means successful? It copies convention of enc_status_change_finish(). I don't think we need anything more than binary pass/fail. We can change it in the future if needed. -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() [not found] <20230525225847.28592-1-kirill.shutemov@linux.intel.com> 2023-05-25 22:58 ` [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail Kirill A. Shutemov @ 2023-05-25 22:58 ` Kirill A. Shutemov 2023-05-25 23:40 ` Sathyanarayanan Kuppuswamy 1 sibling, 1 reply; 7+ messages in thread From: Kirill A. Shutemov @ 2023-05-25 22:58 UTC (permalink / raw) To: dave.hansen, tglx, mingo, bp Cc: decui, rick.p.edgecombe, sathyanarayanan.kuppuswamy, seanjc, thomas.lendacky, x86, linux-kernel, Kirill A. Shutemov, stable Touching privately mapped GPA that is not properly converted to private with MapGPA and accepted leads to unrecoverable exit to VMM. load_unaligned_zeropad() can touch memory that is not owned by the caller, but just happened to next after the owned memory. This load_unaligned_zeropad() behaviour makes it important when kernel asks VMM to convert a GPA from shared to private or back. Kernel must never have a page mapped into direct mapping (and aliases) as private when the GPA is already converted to shared or when GPA is not yet converted to private. guest.enc_status_change_prepare() called before adjusting direct mapping and therefore it is responsible for converting the memory to private. guest.enc_tlb_flush_required() called after adjusting direct mapping and it converts the memory to shared. It is okay to have a shared mapping of memory that is not converted properly. handle_mmio() knows how to deal with load_unaligned_zeropad() stepping on it. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory") Cc: stable@vger.kernel.org --- arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index e146b599260f..84525df750d4 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc) return true; } +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages, + bool enc) +{ + /* + * Only handle shared->private conversion here. + * See the comment in tdx_early_init(). + */ + if (enc) + return tdx_enc_status_changed(vaddr, numpages, enc); + return true; +} + +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages, + bool enc) +{ + /* + * Only handle private->shared conversion here. + * See the comment in tdx_early_init(). + */ + if (!enc) + return tdx_enc_status_changed(vaddr, numpages, enc); + return true; +} + void __init tdx_early_init(void) { u64 cc_mask; @@ -867,9 +891,35 @@ void __init tdx_early_init(void) */ physical_mask &= cc_mask - 1; - x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required; - x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required; - x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed; + /* + * Touching privately mapped GPA that is not properly converted to + * private with MapGPA and accepted leads to unrecoverable exit + * to VMM. + * + * load_unaligned_zeropad() can touch memory that is not owned by + * the caller, but just happened to next after the owned memory. + * This load_unaligned_zeropad() behaviour makes it important when + * kernel asks VMM to convert a GPA from shared to private or back. + * Kernel must never have a page mapped into direct mapping (and + * aliases) as private when the GPA is already converted to shared or + * when GPA is not yet converted to private. + * + * guest.enc_status_change_prepare() called before adjusting direct + * mapping and therefore it is responsible for converting the memory + * to private. + * + * guest.enc_tlb_flush_required() called after adjusting direct mapping + * and it converts the memory to shared. + * + * It is okay to have a shared mapping of memory that is not converted + * properly. handle_mmio() knows how to deal with load_unaligned_zeropad() + * stepping on it. + */ + x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare; + x86_platform.guest.enc_status_change_finish = tdx_enc_status_change_finish; + + x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required; + x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required; pr_info("Guest detected\n"); } -- 2.39.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() 2023-05-25 22:58 ` [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() Kirill A. Shutemov @ 2023-05-25 23:40 ` Sathyanarayanan Kuppuswamy 2023-05-26 11:54 ` Kirill A. Shutemov 0 siblings, 1 reply; 7+ messages in thread From: Sathyanarayanan Kuppuswamy @ 2023-05-25 23:40 UTC (permalink / raw) To: Kirill A. Shutemov, dave.hansen, tglx, mingo, bp Cc: decui, rick.p.edgecombe, seanjc, thomas.lendacky, x86, linux-kernel, stable Hi, On 5/25/23 3:58 PM, Kirill A. Shutemov wrote: > Touching privately mapped GPA that is not properly converted to private > with MapGPA and accepted leads to unrecoverable exit to VMM. > > load_unaligned_zeropad() can touch memory that is not owned by the > caller, but just happened to next after the owned memory. > This load_unaligned_zeropad() behaviour makes it important when kernel > asks VMM to convert a GPA from shared to private or back. Kernel must > never have a page mapped into direct mapping (and aliases) as private > when the GPA is already converted to shared or when GPA is not yet > converted to private. > > guest.enc_status_change_prepare() called before adjusting direct mapping > and therefore it is responsible for converting the memory to private. > > guest.enc_tlb_flush_required() called after adjusting direct mapping and > it converts the memory to shared. Do you mean .enc_status_change_finish() here? Isn't enc_tlb_flush_required() called before adjusting the mapping? > > It is okay to have a shared mapping of memory that is not converted > properly. handle_mmio() knows how to deal with load_unaligned_zeropad() > stepping on it. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory") > Cc: stable@vger.kernel.org > --- > arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 53 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c > index e146b599260f..84525df750d4 100644 > --- a/arch/x86/coco/tdx/tdx.c > +++ b/arch/x86/coco/tdx/tdx.c > @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc) > return true; > } > > +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages, > + bool enc) > +{ > + /* > + * Only handle shared->private conversion here. > + * See the comment in tdx_early_init(). > + */ > + if (enc) > + return tdx_enc_status_changed(vaddr, numpages, enc); > + return true; > +} > + > +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages, > + bool enc) > +{ > + /* > + * Only handle private->shared conversion here. > + * See the comment in tdx_early_init(). > + */ > + if (!enc) > + return tdx_enc_status_changed(vaddr, numpages, enc); > + return true; > +} > + > void __init tdx_early_init(void) > { > u64 cc_mask; > @@ -867,9 +891,35 @@ void __init tdx_early_init(void) > */ > physical_mask &= cc_mask - 1; > > - x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required; > - x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required; > - x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed; > + /* > + * Touching privately mapped GPA that is not properly converted to > + * private with MapGPA and accepted leads to unrecoverable exit > + * to VMM. > + * > + * load_unaligned_zeropad() can touch memory that is not owned by > + * the caller, but just happened to next after the owned memory. > + * This load_unaligned_zeropad() behaviour makes it important when > + * kernel asks VMM to convert a GPA from shared to private or back. > + * Kernel must never have a page mapped into direct mapping (and > + * aliases) as private when the GPA is already converted to shared or > + * when GPA is not yet converted to private. > + * > + * guest.enc_status_change_prepare() called before adjusting direct > + * mapping and therefore it is responsible for converting the memory > + * to private. > + * > + * guest.enc_tlb_flush_required() called after adjusting direct mapping > + * and it converts the memory to shared. Same as above. Is it .enc_status_change_finish() here? > + * > + * It is okay to have a shared mapping of memory that is not converted > + * properly. handle_mmio() knows how to deal with load_unaligned_zeropad() > + * stepping on it. > + */ > + x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare; > + x86_platform.guest.enc_status_change_finish = tdx_enc_status_change_finish; > + > + x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required; > + x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required; > > pr_info("Guest detected\n"); > } -- Sathyanarayanan Kuppuswamy Linux Kernel Developer ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() 2023-05-25 23:40 ` Sathyanarayanan Kuppuswamy @ 2023-05-26 11:54 ` Kirill A. Shutemov 0 siblings, 0 replies; 7+ messages in thread From: Kirill A. Shutemov @ 2023-05-26 11:54 UTC (permalink / raw) To: Sathyanarayanan Kuppuswamy Cc: dave.hansen, tglx, mingo, bp, decui, rick.p.edgecombe, seanjc, thomas.lendacky, x86, linux-kernel, stable On Thu, May 25, 2023 at 04:40:00PM -0700, Sathyanarayanan Kuppuswamy wrote: > Hi, > > On 5/25/23 3:58 PM, Kirill A. Shutemov wrote: > > Touching privately mapped GPA that is not properly converted to private > > with MapGPA and accepted leads to unrecoverable exit to VMM. > > > > load_unaligned_zeropad() can touch memory that is not owned by the > > caller, but just happened to next after the owned memory. > > This load_unaligned_zeropad() behaviour makes it important when kernel > > asks VMM to convert a GPA from shared to private or back. Kernel must > > never have a page mapped into direct mapping (and aliases) as private > > when the GPA is already converted to shared or when GPA is not yet > > converted to private. > > > > guest.enc_status_change_prepare() called before adjusting direct mapping > > and therefore it is responsible for converting the memory to private. > > > > guest.enc_tlb_flush_required() called after adjusting direct mapping and > > it converts the memory to shared. > > Do you mean .enc_status_change_finish() here? Isn't enc_tlb_flush_required() > called before adjusting the mapping? Yes, I copy-pasted wrong callback :/ -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-26 11:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230525225847.28592-1-kirill.shutemov@linux.intel.com>
2023-05-25 22:58 ` [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail Kirill A. Shutemov
2023-05-25 23:28 ` Sathyanarayanan Kuppuswamy
2023-05-26 2:17 ` Huang, Kai
2023-05-26 11:49 ` kirill.shutemov
2023-05-25 22:58 ` [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad() Kirill A. Shutemov
2023-05-25 23:40 ` Sathyanarayanan Kuppuswamy
2023-05-26 11:54 ` Kirill A. Shutemov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox