linux-kernel - Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <662b4aa037bfd5e8f3653a833b460f18636e2bc1.camel@intel.com>
Date: Tue, 5 Nov 2024 09:20:31 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "seanjc@...gle.com" <seanjc@...gle.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>, "Li, Xiaoyao"
	<xiaoyao.li@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>, "yuan.yao@...ux.intel.com"
	<yuan.yao@...ux.intel.com>
Subject: Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace
 generically

> > 
> 
> I think I prefer Binbin's version, as it forces the caller to provide cui(), i.e.
> makes it harder KVM to fail to handle the backend of the hypercall.

Fine to me.

[...]

> 
> The one thing I don't love about providing a separate cui() is that it means
> duplicating the guts of the completion helper.  Ha!  But we can avoid that by
> adding another macro (untested).
> 
> More macros/helpers is a bit ugly too, but I like the symmetry, and it will
> definitely be easier to maintain.  E.g. if the completion phase needs to pivot
> on the exact hypercall, then we can update common code and don't need to remember
> to go update TDX too.
> 
> If no one objects and/or has a better idea, I'll splice together Binbin's patch
> with this blob, and post a series tomorrow.
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8e8ca6dab2b2..0b0fa9174000 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2179,6 +2179,16 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>         kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>  }
>  
> +#define kvm_complete_hypercall_exit(vcpu, ret_reg)                             \
> +do {                                                                           \
> +       u64 ret = (vcpu)->run->hypercall.ret;                                   \
> +                                                                               \
> +       if (!is_64_bit_mode(vcpu))                                              \
> +               ret = (u32)ret;                                                 \
> +       kvm_##ret_reg##_write(vcpu, ret);                                       \
> +       ++(vcpu)->stat.hypercalls;                                              \
> +} while (0)
> +
>  int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>                               unsigned long a0, unsigned long a1,
>                               unsigned long a2, unsigned long a3,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 425a301911a6..aec79e132d3b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9989,12 +9989,8 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
>  
>  static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>  {
> -       u64 ret = vcpu->run->hypercall.ret;
> +       kvm_complete_hypercall_exit(vcpu, rax);
>  
> -       if (!is_64_bit_mode(vcpu))
> -               ret = (u32)ret;
> -       kvm_rax_write(vcpu, ret);
> -       ++vcpu->stat.hypercalls;
>         return kvm_skip_emulated_instruction(vcpu);
>  }
>  

I think there's one issue here:

I assume macro kvm_complete_hypercall_exit(vcpu, ret_reg) will also be used by
TDX.  The issue is it calls !is_64_bit_mode(vcpu), which has below WARN():

        WARN_ON_ONCE(vcpu->arch.guest_state_protected);     

So IIUC TDX will hit this.

Btw, we have below (kinda) duplicated code in ____kvm_emulate_hypercall() too:

	++vcpu->stat.hypercalls;                                                      
                                                                                                                                                               
        if (!op_64_bit)                                                        
                ret = (u32)ret;                                                
                                                                                                                                                               
        kvm_register_write_raw(vcpu, ret_reg, ret);      

If we add a helper to do above, e.g.,

static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg, 
				        unsigned long ret, bool op_64_bit)
{
	if (!op_64_bit)
		ret = (u32)ret;
	kvm_register_write_raw(vcpu, ret_reg, ret);
	++vcpu->stat.hypercalls;
}

Then we can have

static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
{
	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_RAX,
		vcpu->run->hypercall.ret, is_64_bit_mode(vcpu));

	return kvm_skip_emulated_instruction(vcpu);
}

TDX version can use:

	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_R10,
		vcpu->run->hypercall.ret, true);

And ____kvm_emulate_hypercall() can be:

static int ____kvm_emulate_hypercall(vcpu, ...)
{
	...
out:
	kvm_complete_hypercall_exit(vcpu, ret_reg, ret, op_64_bit);
	return 1;
}