[PATCH 1/2] KVM: x86/xen: Remove redundant NULL check

[PATCH 1/2] KVM: x86/xen: Remove redundant NULL check

[Metin Kaya]

'kvm' cannot be NULL if we are at that point.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: 2fd6df2f2b47 ("KVM: x86/xen: intercept EVTCHNOP_send from
guests")

Signed-off-by: Metin Kaya <metikaya@amazon.co.uk>
---
 arch/x86/kvm/xen.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 280cb5dc7341..f2e09481f633 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1734,8 +1734,7 @@ static int kvm_xen_eventfd_deassign(struct kvm *kvm, u32 port)
 	if (!evtchnfd)
 		return -ENOENT;
 
-	if (kvm)
-		synchronize_srcu(&kvm->srcu);
+	synchronize_srcu(&kvm->srcu);
 	if (!evtchnfd->deliver.port.port)
 		eventfd_ctx_put(evtchnfd->deliver.eventfd.ctx);
 	kfree(evtchnfd);
-- 
2.37.1

[PATCH 2/2] KVM: x86: Introduce kvm_gfn_to_hva_cache_valid()

[Metin Kaya]

It simplifies validation of gfn_to_hva_cache to make it less error prone
per the discussion at
https://lore.kernel.org/all/4e29402770a7a254a1ea8ca8165af641ed0832ed.camel@infradead.org.

Signed-off-by: Metin Kaya <metikaya@amazon.co.uk>
---
 arch/x86/kvm/x86.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 43a6a7efc6ec..07d368dc69ad 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3425,11 +3425,22 @@ void kvm_service_local_tlb_flush_requests(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_service_local_tlb_flush_requests);
 
+static inline bool kvm_gfn_to_hva_cache_valid(struct kvm *kvm,
+					      struct gfn_to_hva_cache *ghc,
+					      gpa_t gpa)
+{
+	struct kvm_memslots *slots = kvm_memslots(kvm);
+
+	return !unlikely(slots->generation != ghc->generation ||
+			 gpa != ghc->gpa ||
+			 kvm_is_error_hva(ghc->hva) ||
+			 !ghc->memslot);
+}
+
 static void record_steal_time(struct kvm_vcpu *vcpu)
 {
 	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
 	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
 	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
 	u64 steal;
 	u32 version;
@@ -3445,11 +3456,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 	if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
-
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
+	if (!kvm_gfn_to_hva_cache_valid(vcpu->kvm, ghc, gpa)) {
 		/* We rely on the fact that it fits in a single page. */
 		BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
 
@@ -4729,7 +4736,6 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 {
 	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
 	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
 	static const u8 preempted = KVM_VCPU_PREEMPTED;
 	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
 
@@ -4756,11 +4762,7 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 	if (unlikely(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
-
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot))
+	if (!kvm_gfn_to_hva_cache_valid(vcpu->kvm, ghc, gpa))
 		return;
 
 	st = (struct kvm_steal_time __user *)ghc->hva;
-- 
2.37.1

Re: [PATCH 1/2] KVM: x86/xen: Remove redundant NULL check

[Sean Christopherson]

Channeling Jim, s/redundant/superfluous.  Redundant would imply there's a !NULL
check in close proximity.

On Fri, Sep 09, 2022, Metin Kaya wrote:
> 'kvm' cannot be NULL if we are at that point.

Please make the changelog a standalone thing, i.e. don't rely on the shortlog to
provide context.  Some subsystems prefer making the changelog a continuation of
the shortlog, but IMO it makes life unnecessarily painful for reviewers and
archaeologists, e.g. I don't even see the subject/shortlog right now.

Please elaborate on what guarantees that this in the changelog.  It's trivial to
see from the code, but the fact that it's trivial to document is all the more
reason to provide a one-liner.

E.g.

  Remove a superfluous check that @kvm is non-NULL in
  kvm_xen_eventfd_deassign(), the pointer has already been dereferenced
  multiple times before the check, and the sole caller unconditionally
  passes in a valid pointer.

> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: 2fd6df2f2b47 ("KVM: x86/xen: intercept EVTCHNOP_send from
> guests")

Don't wrap fixes tags.

> Signed-off-by: Metin Kaya <metikaya@amazon.co.uk>
> ---
>  arch/x86/kvm/xen.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 280cb5dc7341..f2e09481f633 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -1734,8 +1734,7 @@ static int kvm_xen_eventfd_deassign(struct kvm *kvm, u32 port)
>  	if (!evtchnfd)
>  		return -ENOENT;
>  
> -	if (kvm)
> -		synchronize_srcu(&kvm->srcu);
> +	synchronize_srcu(&kvm->srcu);
>  	if (!evtchnfd->deliver.port.port)
>  		eventfd_ctx_put(evtchnfd->deliver.eventfd.ctx);
>  	kfree(evtchnfd);
> -- 
> 2.37.1
> 

Re: [PATCH 2/2] KVM: x86: Introduce kvm_gfn_to_hva_cache_valid()

[Sean Christopherson]

On Fri, Sep 09, 2022, Metin Kaya wrote:
> It simplifies validation of gfn_to_hva_cache to make it less error prone

Avoid pronouns as they're ambiguous, e.g. does "it" mean the helper or the patch?
Obviously not a big deal in this case, but avoid pronouns is a good habit to get
into.

> per the discussion at
> https://lore.kernel.org/all/4e29402770a7a254a1ea8ca8165af641ed0832ed.camel@infradead.org.

Please write a proper changelog.  Providing a link to the discussion is wonderful,
but it's a supplement to a good changelog, not a substitution.

> Signed-off-by: Metin Kaya <metikaya@amazon.co.uk>
> ---
>  arch/x86/kvm/x86.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 43a6a7efc6ec..07d368dc69ad 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3425,11 +3425,22 @@ void kvm_service_local_tlb_flush_requests(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_service_local_tlb_flush_requests);
>  
> +static inline bool kvm_gfn_to_hva_cache_valid(struct kvm *kvm,

Don't add inline to local static functions, let the compiler make those decisions.

> +					      struct gfn_to_hva_cache *ghc,
> +					      gpa_t gpa)
> +{
> +	struct kvm_memslots *slots = kvm_memslots(kvm);
> +
> +	return !unlikely(slots->generation != ghc->generation ||

I don't I don't think the unlikely should be here, it's the context in which the
helper is used that determines whether or not the outcome is unlikely.

> +			 gpa != ghc->gpa ||
> +			 kvm_is_error_hva(ghc->hva) ||
> +			 !ghc->memslot);

Might be worth opportunistically reordering the checks to avoid grabbing memslots
when something else is invalid, e.g.

	return gpa != ghc->gpa || kvm_is_error_hva(ghc->hva) || !ghc->memslot ||
	       kvm_memslots(kvm)->generation != ghc->generation);

> +}
> +