Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Wed, 15 Nov 2023 07:09:15 -0800
In-Reply-To: <9395d416-cc5c-536d-641e-ffd971b682d1@gmail.com>
Mime-Version: 1.0
References: <20231110235528.1561679-1-seanjc@google.com> <20231110235528.1561679-7-seanjc@google.com>
 <ffec2e93-cdb1-25e2-06ec-deccf8727ce4@gmail.com> <ZVN6w2Kc2AUmIiJO@google.com>
 <9395d416-cc5c-536d-641e-ffd971b682d1@gmail.com>
Message-ID: <ZVTfG6mARiyttuKj@google.com>
Subject: Re: [PATCH 6/9] KVM: x86: Update guest cpu_caps at runtime for
 dynamic CPUID-based features
From:   Sean Christopherson <seanjc@google.com>
To:     Robert Hoo <robert.hoo.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Maxim Levitsky <mlevitsk@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.