* [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
@ 2022-08-04 13:28 Paolo Bonzini
2022-08-04 13:56 ` Vitaly Kuznetsov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Bonzini @ 2022-08-04 13:28 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Dave Young, Xiaoying Yan, Dr . David Alan Gilbert,
David Woodhouse, stable
Commit 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time
/ preempted status", 2021-11-11) open coded the previous call to
kvm_map_gfn, but in doing so it dropped the comparison between the cached
guest physical address and the one in the MSR. This cause an incorrect
cache hit if the guest modifies the steal time address while the memslots
remain the same. This can happen with kexec, in which case the steal
time data is written at the address used by the old kernel instead of
the old one.
While at it, rename the variable from gfn to gpa since it is a plain
physical address and not a right-shifted one.
Reported-by: Dave Young <ruyang@redhat.com>
Reported-by: Xiaoying Yan <yiyan@redhat.com>
Analyzed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@vger.kernel.org
Fixes: 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/kvm/x86.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e5fa335a4ea7..36dcf18b04bf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3380,6 +3380,7 @@ 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;
@@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
slots = kvm_memslots(vcpu->kvm);
if (unlikely(slots->generation != ghc->generation ||
+ gpa != ghc->gpa ||
kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
- gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
-
/* We rely on the fact that it fits in a single page. */
BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
- if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
+ if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
kvm_is_error_hva(ghc->hva) || !ghc->memslot)
return;
}
--
2.37.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
2022-08-04 13:28 [PATCH] KVM: x86: revalidate steal time cache if MSR value changes Paolo Bonzini
@ 2022-08-04 13:56 ` Vitaly Kuznetsov
2022-08-04 17:08 ` Dr. David Alan Gilbert
2022-08-04 15:54 ` Dr. David Alan Gilbert
2022-08-08 12:42 ` [EXTERNAL] " David Woodhouse
2 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2022-08-04 13:56 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm
Cc: Dave Young, Xiaoying Yan, Dr . David Alan Gilbert,
David Woodhouse, stable
Paolo Bonzini <pbonzini@redhat.com> writes:
> Commit 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time
> / preempted status", 2021-11-11) open coded the previous call to
> kvm_map_gfn, but in doing so it dropped the comparison between the cached
> guest physical address and the one in the MSR. This cause an incorrect
> cache hit if the guest modifies the steal time address while the memslots
> remain the same. This can happen with kexec, in which case the steal
> time data is written at the address used by the old kernel instead of
> the old one.
>
> While at it, rename the variable from gfn to gpa since it is a plain
> physical address and not a right-shifted one.
>
> Reported-by: Dave Young <ruyang@redhat.com>
> Reported-by: Xiaoying Yan <yiyan@redhat.com>
> Analyzed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org
> Fixes: 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/x86.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5fa335a4ea7..36dcf18b04bf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3380,6 +3380,7 @@ 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;
>
> @@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
> slots = kvm_memslots(vcpu->kvm);
>
> if (unlikely(slots->generation != ghc->generation ||
> + gpa != ghc->gpa ||
> kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
> - gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
> -
> /* We rely on the fact that it fits in a single page. */
> BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
>
> - if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
> + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
(It would be nice to somehow get at least a warning when 'gfn_t' is used
instead of 'gpa_t' and vice versa)
> kvm_is_error_hva(ghc->hva) || !ghc->memslot)
> return;
> }
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
2022-08-04 13:56 ` Vitaly Kuznetsov
@ 2022-08-04 17:08 ` Dr. David Alan Gilbert
2022-08-04 17:42 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2022-08-04 17:08 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, linux-kernel, kvm, Dave Young, Xiaoying Yan,
David Woodhouse, stable
* Vitaly Kuznetsov (vkuznets@redhat.com) wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > Commit 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time
> > / preempted status", 2021-11-11) open coded the previous call to
> > kvm_map_gfn, but in doing so it dropped the comparison between the cached
> > guest physical address and the one in the MSR. This cause an incorrect
> > cache hit if the guest modifies the steal time address while the memslots
> > remain the same. This can happen with kexec, in which case the steal
> > time data is written at the address used by the old kernel instead of
> > the old one.
> >
> > While at it, rename the variable from gfn to gpa since it is a plain
> > physical address and not a right-shifted one.
> >
> > Reported-by: Dave Young <ruyang@redhat.com>
> > Reported-by: Xiaoying Yan <yiyan@redhat.com>
> > Analyzed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: David Woodhouse <dwmw@amazon.co.uk>
> > Cc: stable@vger.kernel.org
> > Fixes: 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status")
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > arch/x86/kvm/x86.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e5fa335a4ea7..36dcf18b04bf 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3380,6 +3380,7 @@ 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;
> >
> > @@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
> > slots = kvm_memslots(vcpu->kvm);
> >
> > if (unlikely(slots->generation != ghc->generation ||
> > + gpa != ghc->gpa ||
> > kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
> > - gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
> > -
> > /* We rely on the fact that it fits in a single page. */
> > BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
> >
> > - if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
> > + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
>
> (It would be nice to somehow get at least a warning when 'gfn_t' is used
> instead of 'gpa_t' and vice versa)
Can't sparse be taught to do that?
Dave
> > kvm_is_error_hva(ghc->hva) || !ghc->memslot)
> > return;
> > }
>
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> --
> Vitaly
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
2022-08-04 17:08 ` Dr. David Alan Gilbert
@ 2022-08-04 17:42 ` Sean Christopherson
0 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2022-08-04 17:42 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Vitaly Kuznetsov, Paolo Bonzini, linux-kernel, kvm, Dave Young,
Xiaoying Yan, David Woodhouse, stable
On Thu, Aug 04, 2022, Dr. David Alan Gilbert wrote:
> * Vitaly Kuznetsov (vkuznets@redhat.com) wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > > - gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
> > > -
> > > /* We rely on the fact that it fits in a single page. */
> > > BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
> > >
> > > - if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
> > > + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
> >
> > (It would be nice to somehow get at least a warning when 'gfn_t' is used
> > instead of 'gpa_t' and vice versa)
>
> Can't sparse be taught to do that?
Hmm, it probably could, but the result would likely be a mess. E.g. anything that
shifts the GPA on-demand will require explicit casts to make sparse happy.
This particular case is solvable without sparse, e.g. WARN if gpa[11:0]!=0, or
even better rework the function to actually take a @gfn and then WARN if the
incoming gfn would yield an illegal gpa.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
2022-08-04 13:28 [PATCH] KVM: x86: revalidate steal time cache if MSR value changes Paolo Bonzini
2022-08-04 13:56 ` Vitaly Kuznetsov
@ 2022-08-04 15:54 ` Dr. David Alan Gilbert
2022-08-08 12:42 ` [EXTERNAL] " David Woodhouse
2 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2022-08-04 15:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, Dave Young, Xiaoying Yan, David Woodhouse,
stable
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Commit 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time
> / preempted status", 2021-11-11) open coded the previous call to
> kvm_map_gfn, but in doing so it dropped the comparison between the cached
> guest physical address and the one in the MSR. This cause an incorrect
> cache hit if the guest modifies the steal time address while the memslots
> remain the same. This can happen with kexec, in which case the steal
> time data is written at the address used by the old kernel instead of
> the old one.
>
> While at it, rename the variable from gfn to gpa since it is a plain
> physical address and not a right-shifted one.
>
> Reported-by: Dave Young <ruyang@redhat.com>
> Reported-by: Xiaoying Yan <yiyan@redhat.com>
> Analyzed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
The kernel you built with this in passes Dave Young's kexec set I was
using to debug it, so:
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org
> Fixes: 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/x86.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5fa335a4ea7..36dcf18b04bf 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3380,6 +3380,7 @@ 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;
>
> @@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
> slots = kvm_memslots(vcpu->kvm);
>
> if (unlikely(slots->generation != ghc->generation ||
> + gpa != ghc->gpa ||
> kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
> - gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
> -
> /* We rely on the fact that it fits in a single page. */
> BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
>
> - if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
> + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
> kvm_is_error_hva(ghc->hva) || !ghc->memslot)
> return;
> }
> --
> 2.37.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [EXTERNAL] [PATCH] KVM: x86: revalidate steal time cache if MSR value changes
2022-08-04 13:28 [PATCH] KVM: x86: revalidate steal time cache if MSR value changes Paolo Bonzini
2022-08-04 13:56 ` Vitaly Kuznetsov
2022-08-04 15:54 ` Dr. David Alan Gilbert
@ 2022-08-08 12:42 ` David Woodhouse
2 siblings, 0 replies; 6+ messages in thread
From: David Woodhouse @ 2022-08-08 12:42 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm, Metin Kaya
Cc: Dave Young, Xiaoying Yan, Dr . David Alan Gilbert, stable
[-- Attachment #1: Type: text/plain, Size: 819 bytes --]
On Thu, 2022-08-04 at 15:28 +0200, Paolo Bonzini wrote:
> @@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)=
> slots = kvm_memslots(vcpu->kvm);
>
> if (unlikely(slots->generation != ghc->generation ||
> + gpa != ghc->gpa ||
> kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
If we have to fix the same bug in two open-coded versions of the same
thing then surely it's time for something like...
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);
}
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-08 12:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-04 13:28 [PATCH] KVM: x86: revalidate steal time cache if MSR value changes Paolo Bonzini
2022-08-04 13:56 ` Vitaly Kuznetsov
2022-08-04 17:08 ` Dr. David Alan Gilbert
2022-08-04 17:42 ` Sean Christopherson
2022-08-04 15:54 ` Dr. David Alan Gilbert
2022-08-08 12:42 ` [EXTERNAL] " David Woodhouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).