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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cef7b663-bc6d-44a1-9d5e-736aa097ea68@linux.intel.com>
Date: Wed, 6 Nov 2024 16:32:33 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: "Huang, Kai" <kai.huang@...el.com>, "seanjc@...gle.com"
 <seanjc@...gle.com>
Cc: "Edgecombe, Rick P" <rick.p.edgecombe@...el.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




On 11/5/2024 5:20 PM, Huang, Kai wrote:
>> 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;
> }
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.


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