On Wed, Nov 15, 2023, Robert Hoo wrote:
> On 11/14/2023 9:48 PM, Sean Christopherson wrote:
> > On Mon, Nov 13, 2023, Robert Hoo wrote:
> ...
> > > u32 *caps = vcpu->arch.cpu_caps;
> > > and update guest_cpu_cap_set(), guest_cpu_cap_clear(),
> > > guest_cpu_cap_change() and guest_cpu_cap_restrict() to pass in
> > > vcpu->arch.cpu_caps instead of vcpu, since all of them merely refer to vcpu
> > > cap, rather than whole vcpu info.
> >
> > No, because then every caller would need extra code to pass
> > vcpu->cpu_caps,
>
> Emm, I don't understand this. I tried to modified and compiled, all need to
> do is simply substitute "vcpu" with "vcpu->arch.cpu_caps" in calling. (at
> the end is my diff based on this patch set)
Yes, and I'm saying that
guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER);
guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD);
guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF);
guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI);
is harder to read and write than this
guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PAUSEFILTER);
guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PFTHRESHOLD);
guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VGIF);
guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VNMI);
a one-time search-replace is easy, but the extra boilerplate has a non-zero cost
for every future developer/reader.
> > and passing 'u32 *' provides less type safety than 'struct kvm_vcpu *'.
> > That tradeoff isn't worth making this one path slightly easier to read.
>
> My point is also from vulnerability, long term, since as a principle, we'd
> better pass in param/info to a function of its necessity.
Attempting to apply the principle of least privilege to low level C helpers is
nonsensical. E.g. the helper can trivially get at the owning vcpu via container_of()
(well, if not for typeof assertions not playing nice with arrays, but open coding
container_of() is also trivial and illustrates the point).
struct kvm_vcpu_arch *arch = (void *)caps - offsetof(struct kvm_vcpu_arch, cpu_caps);
struct kvm_vcpu *vcpu = container_of(arch, struct kvm_vcpu, arch);
if (!kvm_cpu_cap_has(x86_feature))
guest_cpu_cap_clear(vcpu, x86_feature);
And the intent behind that principle is to improve security/robustness; what I'm
saying is that passing in a 'u32 *" makes the overall implementation _less_ robust,
as it opens up the possibilities of passing in an unsafe/incorrect pointer. E.g.
a well-intentioned, not _that_ obviously broken example is:
guest_cpu_cap_restrict(&vcpu->arch.cpu_caps[CPUID_1_ECX], X86_FEATURE_XSAVE);
> e.g. cpuid_entry2_find().
The main reason cpuid_entry2_find() exists is because KVM checks the incoming
array provided by KVM_SET_CPUID2, which is also the reason why
__kvm_update_cpuid_runtime() takes an @entries array instead of just @vcpu.
On 11/15/2023 11:09 PM, Sean Christopherson wrote:
...
>>> No, because then every caller would need extra code to pass
>>> vcpu->cpu_caps,
>>
>> Emm, I don't understand this. I tried to modified and compiled, all need to
>> do is simply substitute "vcpu" with "vcpu->arch.cpu_caps" in calling. (at
>> the end is my diff based on this patch set)
>
> Yes, and I'm saying that
>
> guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER);
> guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD);
> guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF);
> guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI);
>
> is harder to read and write than this
>
> guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PAUSEFILTER);
> guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PFTHRESHOLD);
> guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VGIF);
> guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VNMI);
>
> a one-time search-replace is easy, but the extra boilerplate has a non-zero cost
> for every future developer/reader.
Hmm, I think this is trivial. And can be solved/eased by other means, e.g.
Macro?. Rather than in the sacrifice of letting function's inside (easily)
access those info it shouldn't.
>
>>> and passing 'u32 *' provides less type safety than 'struct kvm_vcpu *'.
>>> That tradeoff isn't worth making this one path slightly easier to read.
>>
>> My point is also from vulnerability, long term, since as a principle, we'd
>> better pass in param/info to a function of its necessity.
>
> Attempting to apply the principle of least privilege to low level C helpers is
> nonsensical. E.g. the helper can trivially get at the owning vcpu via container_of()
> (well, if not for typeof assertions not playing nice with arrays, but open coding
> container_of() is also trivial and illustrates the point).
>
> struct kvm_vcpu_arch *arch = (void *)caps - offsetof(struct kvm_vcpu_arch, cpu_caps);
> struct kvm_vcpu *vcpu = container_of(arch, struct kvm_vcpu, arch);
>
> if (!kvm_cpu_cap_has(x86_feature))
> guest_cpu_cap_clear(vcpu, x86_feature);
>
> And the intent behind that principle is to improve security/robustness; what I'm
> saying is that passing in a 'u32 *" makes the overall implementation _less_ robust,
> as it opens up the possibilities of passing in an unsafe/incorrect pointer. E.g.
> a well-intentioned, not _that_ obviously broken example is:
>
> guest_cpu_cap_restrict(&vcpu->arch.cpu_caps[CPUID_1_ECX], X86_FEATURE_XSAVE);
>
>> e.g. cpuid_entry2_find().
>
> The main reason cpuid_entry2_find() exists is because KVM checks the incoming
> array provided by KVM_SET_CPUID2, which is also the reason why
> __kvm_update_cpuid_runtime() takes an @entries array instead of just @vcpu.
Thanks for detailed explanation, I understand your points deeper, though I would
still prefer to honoring the principle if it was me to write the function. The
concerns above can/should be addressed by other means. (If some really cannot be
solved in C, i.e. more stringent type check, it's C to blame ;) but it on the
other side offers those flexibility that other languages cannot, doesn't it?)
Another pros of the principle is that, it's also a fence, prevent (at least
raise the bar) people in the future from doing something that shouldn't be in
the function, e.g. for his convenience to quickly fix a bug etc.
Anyway, it's a dilemma, and I said it's a less important point for this great
progress of vCPUID's implementation, thanks.
Reviewed-by: Robert Hoo <[email protected]>