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

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

[Vitaly Kuznetsov]

Li RongQing <lirongqing@baidu.com> writes:

> When set_memory_decrypted() fails, pages may be left fully or partially
> decrypted. before free the pages to return pool, it should be encypted via
> set_memory_encrypted(), and if encryption fails, leak the pages

Out of curiosity,

shouldn't we rather try to make set_memory_decrypted() more atomic to
avoid the need to hunt down all users of the API? E.g. in Hyper-V's
__vmbus_establish_gpadl() I see:

     ret = set_memory_decrypted((unsigned long)kbuffer,
                                PFN_UP(size));
     if (ret) {
             dev_warn(&channel->device_obj->device,
	     ...

doesn't it have the exact same issue you're trying to address for kvmclock?

>
> 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;

-- 
Vitaly

RE: [外部邮件] Re: [PATCH] x86/kvm: fix the decrypted pages free in kvmclock

[Li,Rongqing]

> 
> 
> Out of curiosity,
> 
> shouldn't we rather try to make set_memory_decrypted() more atomic to avoid
> the need to hunt down all users of the API? E.g. in Hyper-V's
> __vmbus_establish_gpadl() I see:
> 
>      ret = set_memory_decrypted((unsigned long)kbuffer,
>                                 PFN_UP(size));
>      if (ret) {
>              dev_warn(&channel->device_obj->device,
> 	     ...
> 
> doesn't it have the exact same issue you're trying to address for kvmclock?
> 

This patch should show the reason

https://lkml.org/lkml/2023/10/24/1369

thanks

-Li