[PATCH 0/2] x86/tdx: Fix one more load_unaligned_zeropad() issue

[PATCH 0/2] x86/tdx: Fix one more load_unaligned_zeropad() issue

[Kirill A. Shutemov]

During review of TDX guests on Hyper-V patchset Dave pointed to the
potential race between changing page private/shared status and
load_unaligned_zeropad().

Fix the issue.

Kirill A. Shutemov (2):
  x86/mm: Allow guest.enc_status_change_prepare() to fail
  x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad()

 arch/x86/coco/tdx/tdx.c         | 56 +++++++++++++++++++++++++++++++--
 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 +-
 5 files changed, 61 insertions(+), 8 deletions(-)

-- 
2.39.3

[PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail

[Kirill A. Shutemov]

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

[PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad()

[Kirill A. Shutemov]

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

Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail

[Sathyanarayanan Kuppuswamy]

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

Re: [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad()

[Sathyanarayanan Kuppuswamy]

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

Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail

[Huang, Kai]

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?

RE: [PATCH 0/2] x86/tdx: Fix one more load_unaligned_zeropad() issue

[Dexuan Cui]

> From: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Sent: Thursday, May 25, 2023 3:59 PM
>  ...
> During review of TDX guests on Hyper-V patchset Dave pointed to the
> potential race between changing page private/shared status and
> load_unaligned_zeropad().
> 
> Fix the issue.

Hi Kirill, I tested the patches on Hyper-V, and didn't find any issue.

Thanks for the patches! It looks like the patches can address Dave's
concerns. I'll wait for your patches to go in first, and then I'll rebase
my patches.

Thanks,
Dexuan

Re: [PATCH 1/2] x86/mm: Allow guest.enc_status_change_prepare() to fail

[kirill.shutemov]

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

Re: [PATCH 2/2] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad()

[Kirill A. Shutemov]

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