linux-kernel - Re: [PATCH v2 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: <6de48ba0-2e63-4559-a6b9-1b75ab5d712d@intel.com>
Date: Wed, 14 Aug 2024 14:36:32 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, Isaku Yamahata
	<isaku.yamahata@...el.com>
CC: Binbin Wu <binbin.wu@...ux.intel.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <pbonzini@...hat.com>,
	<rick.p.edgecombe@...el.com>, <michael.roth@....com>
Subject: Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace
 generically


>>>>> +{
>>>>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>>>>> +		return false;
>>>>
>>>> Is this to detect potential bug? Maybe
>>>> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>>>>                !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
>>>> Overkill?
>>>
>>> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
>>> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
> 
> KVM does use __builtin_constant_p() to effectively disable some assertions when
> it's allowed (by KVM's arbitrary rules) to pass in a non-constant value.  E.g.
> see all the vmcs_checkNN() helpers.  If we didn't waive the assertion for values
> that aren't constant at compile-time, all of the segmentation code would need to
> be unwound into switch statements.

Yeah I saw vmcs_checkNN(), but I think __builtin_constant_p() makes 
sense for vmcs_checkNN()s because they are widely called.  But 
is_kvm_hc_exit_enabled() doesn't seem so.  But no hard opinion here.  As 
you said, it's kinda overkill (or abused to use) but zero-generated code.

> 
> But for things like guest_cpuid_has(), the rule is that the input must be a
> compile-time constant.
> 
>>> IIUC you need some build time guarantee here, but __builtin_constant_p() can
>>> return false, in which case the above BUILD_BUG_ON() does nothing, which
>>> defeats the purpose.
>>
>> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
>> can detect the usage in the patch 2/2,
>> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
>> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
>>
>> Although this version doesn't help for the one in kvm_emulate_hypercall(),
>> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
>>
>> Maybe we can just drop this WARN_ON_ONCE().
> 
> Yeah, I think it makes sense to drop the WARN, otherwise I suspect we'll end up
> dancing around the helper just to avoid the warning.

Agreed, given @nr is from guest.