| 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
| ||
|
Message-ID: <57d308a8-0ce5-41f8-aef4-33d36723c434@linux.intel.com>
Date: Mon, 25 Nov 2024 14:47:23 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>, Kai Huang <kai.huang@...el.com>
Cc: Xiaoyao Li <xiaoyao.li@...el.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"yuan.yao@...ux.intel.com" <yuan.yao@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rick P Edgecombe <rick.p.edgecombe@...el.com>,
Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace
generically
On 11/6/2024 11:30 PM, Sean Christopherson wrote:
> On Wed, Nov 06, 2024, Binbin Wu wrote:
>> On 11/6/2024 4:54 PM, Huang, Kai wrote:
>>> On Wed, 2024-11-06 at 16:32 +0800, Binbin Wu wrote:
>>>>> 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;
>>>>> }
>>>> If this is going to be the final version, it would be better to make it
>>>> public, and export the symbol, so that TDX code can reuse it.
>>> Does making it 'static inline' and moving to kvm_host.h work?
>> It doesn't have a complete definition of struct kvm_vcpu in
>> arch/x86/include/asm/kvm_host.h, and the code is dereferencing
>> struct kvm_vcpu.
>> Also, the definition of kvm_register_write_raw() is in
>> arch/x86/kvm/kvm_cache_regs.h, which make it difficult to be called
>> there.
> A way around that would be to move the declarations from asm/kvm_host.h to x86.h,
> and then kvm_complete_hypercall_exit() can be inlined (or not), without having to
> deal with the kvm_host.h ordering issues.
>
> IMO, KVM x86 would ideally put as much as possible in x86.h. The vast majority
> of KVM x86's exports are intended only for the vendor modules. Declaring those
> exports in kvm_host.h is effectively bleeding KVM internals to the broader kernel.
>
> I'll go that route for the series, assuming it works as I intend :-)
>
Based on the previous discussions, is the below code expected?
If so, I am gonna use __kvm_emulate_hypercall() and kvm_complete_hypercall_exit()
in TDX code for handling KVM hypercalls.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e51a95aba824..356620f74bb0 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2205,10 +2205,6 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
}
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
- unsigned long a0, unsigned long a1,
- unsigned long a2, unsigned long a3,
- int op_64_bit, int cpl);
int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2b5b0ae3dd7e..05191c4af156 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10008,19 +10008,17 @@ 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, VCPU_REGS_RAX,
+ vcpu->run->hypercall.ret, is_64_bit_mode(vcpu));
- if (!is_64_bit_mode(vcpu))
- ret = (u32)ret;
- kvm_rax_write(vcpu, ret);
- ++vcpu->stat.hypercalls;
return kvm_skip_emulated_instruction(vcpu);
}
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
- unsigned long a0, unsigned long a1,
- unsigned long a2, unsigned long a3,
- int op_64_bit, int cpl)
+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+ unsigned long a0, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ int op_64_bit, int cpl, int ret_reg,
+ int (*cui)(struct kvm_vcpu *vcpu))
{
unsigned long ret;
@@ -10104,16 +10102,15 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
}
out:
- ++vcpu->stat.hypercalls;
- return ret;
+ kvm_complete_hypercall_exit(vcpu, ret_reg, ret, op_64_bit);
+
+ return 1;
}
-EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
+EXPORT_SYMBOL_GPL(____kvm_emulate_hypercall);
int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
{
- unsigned long nr, a0, a1, a2, a3, ret;
- int op_64_bit;
- int cpl;
+ int r;
if (kvm_xen_hypercall_enabled(vcpu->kvm))
return kvm_xen_hypercall(vcpu);
@@ -10121,23 +10118,13 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
if (kvm_hv_hypercall_enabled(vcpu))
return kvm_hv_hypercall(vcpu);
return kvm_hv_hypercall(vcpu);
- nr = kvm_rax_read(vcpu);
- a0 = kvm_rbx_read(vcpu);
- a1 = kvm_rcx_read(vcpu);
- a2 = kvm_rdx_read(vcpu);
- a3 = kvm_rsi_read(vcpu);
- op_64_bit = is_64_bit_hypercall(vcpu);
- cpl = kvm_x86_call(get_cpl)(vcpu);
-
- ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
- if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
- /* MAP_GPA tosses the request to the user space. */
+ r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
+ is_64_bit_hypercall(vcpu),
+ kvm_x86_call(get_cpl)(vcpu), RAX,
+ complete_hypercall_exit);
+ if (r <= 0)
return 0;
- if (!op_64_bit)
- ret = (u32)ret;
- kvm_rax_write(vcpu, ret);
-
return kvm_skip_emulated_instruction(vcpu);
}
EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a84c48ef5278..5667d029e7f0 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -568,4 +568,25 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
unsigned int port, void *data, unsigned int count,
int in);
+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+ unsigned long a0, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ int op_64_bit, int cpl, int ret_reg,
+ int (*cui)(struct kvm_vcpu *vcpu));
+
+#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret, cui) \
+ ____kvm_emulate_hypercall(vcpu, \
+ kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu), \
+ kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu), \
+ kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret, \
+ cui)
+
+static inline 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;
+}
+
#endif
Powered by blists - more mailing lists