RE: [PATCH v2] x86/mm/cpa: Warn if set_memory_XXcrypted() fails

[Michael Kelley]

From: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx> Sent: Friday, October 27, 2023 2:48 PM
> 
> On TDX it is possible for the untrusted host to cause
> set_memory_encrypted() or set_memory_decrypted() to fail such that an
> error is returned and the resulting memory is shared. Callers need to take care
> to handle these errors to avoid returning decrypted (shared) memory to the
> page allocator, which could lead to functional or security issues.
> In terms of security, the problematic case is guest PTEs mapping the shared
> alias GFNs, since the VMM has control of the shared mapping in the EPT/NPT.
> 
> Such conversion errors may herald future system instability, but are
> temporarily survivable with proper handling in the caller. The kernel
> traditionally makes every effort to keep running, but it is expected that some
> coco guests may prefer to play it safe security-wise, and panic in this case. To
> accommodate both cases, warn when the arch breakouts for converting
> memory at the VMM layer return an error to CPA. Security focused users can
> rely on panic_on_warn to defend against bugs in the callers. Some VMMs are
> not known to behave in the troublesome way, so users that would like to
> terminate on any unusual behavior by the VMM around this will be covered as
> well.
> 
> Since the arch breakouts host the logic for handling coco implementation
> specific errors, an error returned from them means that the set_memory() call
> is out of options for handling the error internally. Make this the condition to
> warn about.
> 
> It is possible that very rarely these functions could fail due to guest memory
> pressure (in the case of failing to allocate a huge page when splitting a page
> table). Don't warn in this case because it is a lot less likely to indicate an attack
> by the host and it is not clear which
> set_memory() calls should get the same treatment. That corner should be
> addressed by future work that considers the more general problem and not
> just papers over a single set_memory() variant.
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> Suggested-by: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> ---
> For v2:
>  - Update commit log to call out importance of PTEs being shared in
>    guest for there to be a problem, and that some users may want to
>    terminate the guest on any unsual behavior. (Michael Kelley)
>  - Remove out label (Thomas Lendacky, Sathyanarayanan Kuppuswamy)
> 
> v1 is here:
> https://lore.kernel.org/lkml/20231024234829.1443125-1-rick.p.edgecombe@xxxxxxxxx/
> ---
>  arch/x86/mm/pat/set_memory.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/mm/pat/set_memory.c
> b/arch/x86/mm/pat/set_memory.c index bda9f129835e..34f2c0c88a6b
> 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -2153,7 +2153,7 @@ static int __set_memory_enc_pgtable(unsigned
> long addr, int numpages, bool enc)
> 
>  	/* Notify hypervisor that we are about to set/clr encryption attribute. */
>  	if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc))
> -		return -EIO;
> +		goto vmm_fail;
> 
>  	ret = __change_page_attr_set_clr(&cpa, 1);
> 
> @@ -2167,12 +2167,19 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
>  	cpa_flush(&cpa, 0);
> 
>  	/* Notify hypervisor that we have successfully set/clr encryption attribute. */
> -	if (!ret) {
> -		if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> -			ret = -EIO;
> -	}
> +	if (ret)
> +		return ret;
> 
> -	return ret;
> +	if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> +		goto vmm_fail;
> +
> +	return 0;
> +
> +vmm_fail:
> +	WARN_ONCE(1, "CPA VMM failure to convert memory (addr=%p, numpages=%d) to %s.\n",
> +		  (void *)addr, numpages, enc ? "private" : "shared");
> +
> +	return -EIO;
>  }
> 
>  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> --
> 2.34.1

Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>