Re: [PATCH][v1] x86/kvm: Fix the decrypted pages free in kvmclock

[Sean Christopherson <seanjc@google.com>]

+more x86 folks

On Wed, Jun 12, 2024, Li RongQing wrote:
> When set_memory_decrypted() fails, pages may be left fully or partially
> decrypted. before free the pages to return pool, it should be encrypted
> via set_memory_encrypted(), or else this could lead to functional or
> security issues, if encrypting the pages fails, leak the pages

That seems like a major flaw in the API, i.e. not something that should be "fixed"
in kvmclock, especially since the vmm_fail paths only WARN.

Commit 82ace185017f ("x86/mm/cpa: Warn for set_memory_XXcrypted() VMM fails")
says the reason for only warning is to be able to play nice with both security
and uptime:

    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.

But punting the issue to the caller doesn't help with that, it just makes it all
too easy to introduce security bugs.  Wouldn't it be better to do something along
the lines of CONFIG_BUG_ON_DATA_CORRUPTION (though maybe runtime configurable?)
and let the end user explicitly decide what to do?

> Fixes: 6a1cac56f41f ("x86/kvm: Use __bss_decrypted attribute in shared variables")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kernel/kvmclock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 5b2c152..5e9f9d2 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -228,7 +228,8 @@ static void __init kvmclock_init_mem(void)
>  		r = set_memory_decrypted((unsigned long) hvclock_mem,
>  					 1UL << order);
>  		if (r) {
> -			__free_pages(p, order);
> +			if (!set_memory_encrypted((unsigned long)hvclock_mem, 1UL << order))
> +				__free_pages(p, order);
>  			hvclock_mem = NULL;
>  			pr_warn("kvmclock: set_memory_decrypted() failed. Disabling\n");
>  			return;
> -- 
> 2.9.4
>