+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@xxxxxxxxx>
> ---
> 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
>
