Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
magic INVALID_GPA, which would also fail the sanity check.
Sean Christopherson (3):
KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
activation
KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
INVALID_GPA
Sean Christopherson (3):
KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
activation
KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
INVALID_GPA
virt/kvm/pfncache.c | 48 ++++++++++++++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 14 deletions(-)
base-commit: 964d0c614c7f71917305a5afdca9178fe8231434
--
2.44.0.291.gc1ea87d7ee-goog
Add a helper to check that the incoming length for a gfn_to_pfn_cache is
valid with respect to the cache's GPA and/or HVA. To avoid activating a
cache with a bogus GPA, a future fix will fork the page split check in
the inner refresh path into activate() and the public rerfresh() APIs, at
which point KVM will check the length in three separate places.
Deliberately keep the "page offset" logic open coded, as the only other
path that consumes the offset, __kvm_gpc_refresh(), already needs to
differentiate between GPA-based and HVA-based caches, and it's not obvious
that using a helper is a net positive in overall code readability.
Note, for GPA-based caches, this has a subtle side effect of using the GPA
instead of the resolved HVA in the check() path, but that should be a nop
as the HVA offset is derived from the GPA, i.e. the two offsets are
identical, barring a KVM bug.
Signed-off-by: Sean Christopherson <[email protected]>
---
virt/kvm/pfncache.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 4e07112a24c2..8f2121b5f2a0 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -57,6 +57,19 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
spin_unlock(&kvm->gpc_lock);
}
+static bool kvm_gpc_is_valid_len(gpa_t gpa, unsigned long uhva,
+ unsigned long len)
+{
+ unsigned long offset = kvm_is_error_gpa(gpa) ? offset_in_page(uhva) :
+ offset_in_page(gpa);
+
+ /*
+ * The cached access must fit within a single page. The 'len' argument
+ * to activate() and refresh() exists only to enforce that.
+ */
+ return offset + len <= PAGE_SIZE;
+}
+
bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
{
struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
@@ -74,7 +87,7 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
if (kvm_is_error_hva(gpc->uhva))
return false;
- if (offset_in_page(gpc->uhva) + len > PAGE_SIZE)
+ if (!kvm_gpc_is_valid_len(gpc->gpa, gpc->uhva, len))
return false;
if (!gpc->valid)
@@ -247,13 +260,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva)))
return -EINVAL;
- /*
- * The cached acces must fit within a single page. The 'len' argument
- * exists only to enforce that.
- */
- page_offset = kvm_is_error_gpa(gpa) ? offset_in_page(uhva) :
- offset_in_page(gpa);
- if (page_offset + len > PAGE_SIZE)
+ if (!kvm_gpc_is_valid_len(gpa, uhva, len))
return -EINVAL;
lockdep_assert_held(&gpc->refresh_lock);
@@ -270,6 +277,8 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
old_uhva = PAGE_ALIGN_DOWN(gpc->uhva);
if (kvm_is_error_gpa(gpa)) {
+ page_offset = offset_in_page(uhva);
+
gpc->gpa = INVALID_GPA;
gpc->memslot = NULL;
gpc->uhva = PAGE_ALIGN_DOWN(uhva);
@@ -279,6 +288,8 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
} else {
struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
+ page_offset = offset_in_page(gpa);
+
if (gpc->gpa != gpa || gpc->generation != slots->generation ||
kvm_is_error_hva(gpc->uhva)) {
gfn_t gfn = gpa_to_gfn(gpa);
--
2.44.0.291.gc1ea87d7ee-goog
When activating a gfn_to_pfn_cache, verify that the offset+length is sane
and usable before marking the cache active. Letting __kvm_gpc_refresh()
detect the problem results in a cache being marked active without setting
the GPA (or any other fields), which in turn results in KVM trying to
refresh a cache with INVALID_GPA.
Attempting to refresh a cache with INVALID_GPA isn't functionally
problematic, but it runs afoul of the sanity check that exactly one of
GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or
HVA-based.
Reported-by: [email protected]
Closes: https://lore.kernel.org/all/[email protected]
Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA")
Cc: David Woodhouse <[email protected]>
Cc: Paul Durrant <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
virt/kvm/pfncache.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 8f2121b5f2a0..91b0e329006b 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -245,8 +245,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
return -EFAULT;
}
-static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva,
- unsigned long len)
+static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva)
{
unsigned long page_offset;
bool unmap_old = false;
@@ -260,9 +259,6 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva)))
return -EINVAL;
- if (!kvm_gpc_is_valid_len(gpa, uhva, len))
- return -EINVAL;
-
lockdep_assert_held(&gpc->refresh_lock);
write_lock_irq(&gpc->lock);
@@ -365,6 +361,9 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
guard(mutex)(&gpc->refresh_lock);
+ if (!kvm_gpc_is_valid_len(gpc->gpa, gpc->uhva, len))
+ return -EINVAL;
+
/*
* If the GPA is valid then ignore the HVA, as a cache can be GPA-based
* or HVA-based, not both. For GPA-based caches, the HVA will be
@@ -372,7 +371,7 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
*/
uhva = kvm_is_error_gpa(gpc->gpa) ? gpc->uhva : KVM_HVA_ERR_BAD;
- return __kvm_gpc_refresh(gpc, gpc->gpa, uhva, len);
+ return __kvm_gpc_refresh(gpc, gpc->gpa, uhva);
}
void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
@@ -392,6 +391,9 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned
{
struct kvm *kvm = gpc->kvm;
+ if (!kvm_gpc_is_valid_len(gpa, uhva, len))
+ return -EINVAL;
+
guard(mutex)(&gpc->refresh_lock);
if (!gpc->active) {
@@ -411,7 +413,7 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned
gpc->active = true;
write_unlock_irq(&gpc->lock);
}
- return __kvm_gpc_refresh(gpc, gpa, uhva, len);
+ return __kvm_gpc_refresh(gpc, gpa, uhva);
}
int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
--
2.44.0.291.gc1ea87d7ee-goog
Explicit disallow activating a gfn_to_pfn_cache with an error gpa, i.e.
INVALID_GPA, to ensure that KVM doesn't mistake a GPA-based cache for an
HVA-based cache (KVM uses INVALID_GPA as a magic value to differentiate
between GPA-based and HVA-based caches).
WARN if KVM attempts to activate a cache with INVALID_GPA, purely so that
new caches need to at least consider what to do with a "bad" GPA, as all
existing usage of kvm_gpc_activate() guarantees gpa != INVALID_GPA. I.e.
removing the WARN in the future is completely reasonable if doing so would
yield cleaner/better code overall.
Signed-off-by: Sean Christopherson <[email protected]>
---
virt/kvm/pfncache.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 91b0e329006b..f618719644e0 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -418,6 +418,13 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned
int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
{
+ /*
+ * Explicitly disallow INVALID_GPA so that the magic value can be used
+ * by KVM to differentiate between GPA-based and HVA-based caches.
+ */
+ if (WARN_ON_ONCE(kvm_is_error_gpa(gpa)))
+ return -EINVAL;
+
return __kvm_gpc_activate(gpc, gpa, KVM_HVA_ERR_BAD, len);
}
--
2.44.0.291.gc1ea87d7ee-goog
On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> Explicit disallow activating a gfn_to_pfn_cache with an error gpa, i.e.
> INVALID_GPA, to ensure that KVM doesn't mistake a GPA-based cache for an
> HVA-based cache (KVM uses INVALID_GPA as a magic value to differentiate
> between GPA-based and HVA-based caches).
>
> WARN if KVM attempts to activate a cache with INVALID_GPA, purely so that
> new caches need to at least consider what to do with a "bad" GPA, as all
> existing usage of kvm_gpc_activate() guarantees gpa != INVALID_GPA. I.e.
> removing the WARN in the future is completely reasonable if doing so would
> yield cleaner/better code overall.
>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> Add a helper to check that the incoming length for a gfn_to_pfn_cache is
> valid with respect to the cache's GPA and/or HVA. To avoid activating a
> cache with a bogus GPA, a future fix will fork the page split check in
> the inner refresh path into activate() and the public rerfresh() APIs, at
> which point KVM will check the length in three separate places.
>
> Deliberately keep the "page offset" logic open coded, as the only other
> path that consumes the offset, __kvm_gpc_refresh(), already needs to
> differentiate between GPA-based and HVA-based caches, and it's not obvious
> that using a helper is a net positive in overall code readability.
>
> Note, for GPA-based caches, this has a subtle side effect of using the GPA
> instead of the resolved HVA in the check() path, but that should be a nop
> as the HVA offset is derived from the GPA, i.e. the two offsets are
> identical, barring a KVM bug.
>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> When activating a gfn_to_pfn_cache, verify that the offset+length is sane
> and usable before marking the cache active. Letting __kvm_gpc_refresh()
> detect the problem results in a cache being marked active without setting
> the GPA (or any other fields), which in turn results in KVM trying to
> refresh a cache with INVALID_GPA.
>
> Attempting to refresh a cache with INVALID_GPA isn't functionally
> problematic, but it runs afoul of the sanity check that exactly one of
> GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or
> HVA-based.
>
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/all/[email protected]
> Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA")
Reviewed-by: David Woodhouse <[email protected]>
On 20/03/2024 00:15, Sean Christopherson wrote:
> Add a helper to check that the incoming length for a gfn_to_pfn_cache is
> valid with respect to the cache's GPA and/or HVA. To avoid activating a
> cache with a bogus GPA, a future fix will fork the page split check in
> the inner refresh path into activate() and the public rerfresh() APIs, at
nit: typo
> which point KVM will check the length in three separate places.
>
> Deliberately keep the "page offset" logic open coded, as the only other
> path that consumes the offset, __kvm_gpc_refresh(), already needs to
> differentiate between GPA-based and HVA-based caches, and it's not obvious
> that using a helper is a net positive in overall code readability.
>
> Note, for GPA-based caches, this has a subtle side effect of using the GPA
> instead of the resolved HVA in the check() path, but that should be a nop
> as the HVA offset is derived from the GPA, i.e. the two offsets are
> identical, barring a KVM bug.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> virt/kvm/pfncache.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
Reviewed-by: Paul Durrant <[email protected]>
On 20/03/2024 00:15, Sean Christopherson wrote:
> When activating a gfn_to_pfn_cache, verify that the offset+length is sane
> and usable before marking the cache active. Letting __kvm_gpc_refresh()
> detect the problem results in a cache being marked active without setting
> the GPA (or any other fields), which in turn results in KVM trying to
> refresh a cache with INVALID_GPA.
>
> Attempting to refresh a cache with INVALID_GPA isn't functionally
> problematic, but it runs afoul of the sanity check that exactly one of
> GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or
> HVA-based.
>
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/all/[email protected]
> Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA")
> Cc: David Woodhouse <[email protected]>
> Cc: Paul Durrant <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> virt/kvm/pfncache.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
Reviewed-by: Paul Durrant <[email protected]>
On 20/03/2024 00:15, Sean Christopherson wrote:
> Explicit disallow activating a gfn_to_pfn_cache with an error gpa, i.e.
> INVALID_GPA, to ensure that KVM doesn't mistake a GPA-based cache for an
> HVA-based cache (KVM uses INVALID_GPA as a magic value to differentiate
> between GPA-based and HVA-based caches).
>
> WARN if KVM attempts to activate a cache with INVALID_GPA, purely so that
> new caches need to at least consider what to do with a "bad" GPA, as all
> existing usage of kvm_gpc_activate() guarantees gpa != INVALID_GPA. I.e.
> removing the WARN in the future is completely reasonable if doing so would
> yield cleaner/better code overall.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> virt/kvm/pfncache.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
Reviewed-by: Paul Durrant <[email protected]>
On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
> marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
> other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
> magic INVALID_GPA, which would also fail the sanity check.
>
> Sean Christopherson (3):
> KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
> KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
> activation
> KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
> INVALID_GPA
It looks like these conflict with
https://lore.kernel.org/kvm/[email protected]/
Want to arrange them to come after it?
On Fri, Mar 22, 2024, David Woodhouse wrote:
> On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> > Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
> > marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
> > other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
> > magic INVALID_GPA, which would also fail the sanity check.
> >
> > Sean Christopherson (3):
> > KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
> > KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
> > activation
> > KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
> > INVALID_GPA
>
> It looks like these conflict with
> https://lore.kernel.org/kvm/[email protected]/
>
> Want to arrange them to come after it?
Very belated, yes. Though by the time you read this, they should be in
kvm-x86/next.
On Tue, 19 Mar 2024 17:15:39 -0700, Sean Christopherson wrote:
> Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
> marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
> other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
> magic INVALID_GPA, which would also fail the sanity check.
>
> Sean Christopherson (3):
> KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
> KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
> activation
> KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
> INVALID_GPA
>
> [...]
Applied to kvm-x86 fixes, thanks!
[1/3] KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
https://github.com/kvm-x86/linux/commit/18f06e976925
[2/3] KVM: Check validity of offset+length of gfn_to_pfn_cache prior to activation
https://github.com/kvm-x86/linux/commit/5c9ca4ed8908
[3/3] KVM: Explicitly disallow activatating a gfn_to_pfn_cache with INVALID_GPA
https://github.com/kvm-x86/linux/commit/fc62a4e8dee2
--
https://github.com/kvm-x86/linux/tree/next
On Mon, 2024-04-08 at 16:21 -0700, Sean Christopherson wrote:
> On Fri, Mar 22, 2024, David Woodhouse wrote:
> > On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> > > Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
> > > marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
> > > other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
> > > magic INVALID_GPA, which would also fail the sanity check.
> > >
> > > Sean Christopherson (3):
> > > KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
> > > KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
> > > activation
> > > KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
> > > INVALID_GPA
> >
> > It looks like these conflict with
> > https://lore.kernel.org/kvm/[email protected]/
> >
> > Want to arrange them to come after it?
>
> Very belated, yes. Though by the time you read this, they should be in
> kvm-x86/next.
Did that 'yes' mean 'no'? Because your three patches are in, but you
didn't arrange them to come after my 'clean up rwlock abuse' patch, as
you seemed to be saying 'yes' to...
On Tue, Apr 09, 2024, David Woodhouse wrote:
> On Mon, 2024-04-08 at 16:21 -0700, Sean Christopherson wrote:
> > On Fri, Mar 22, 2024, David Woodhouse wrote:
> > > On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote:
> > > > Fix a bug found by syzkaller, thanks to a new WARN sanity check, where KVM
> > > > marks a gfn_to_pfn_cache as active without actually setting gpc->gpa or any
> > > > other metadata. On top, harden against _directly_ setting gpc->gpa to KVM's
> > > > magic INVALID_GPA, which would also fail the sanity check.
> > > >
> > > > Sean Christopherson (3):
> > > > KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
> > > > KVM: Check validity of offset+length of gfn_to_pfn_cache prior to
> > > > activation
> > > > KVM: Explicitly disallow activatating a gfn_to_pfn_cache with
> > > > INVALID_GPA
> > >
> > > It looks like these conflict with
> > > https://lore.kernel.org/kvm/[email protected]/
> > >
> > > Want to arrange them to come after it?
> >
> > Very belated, yes. Though by the time you read this, they should be in
> > kvm-x86/next.
>
> Did that 'yes' mean 'no'? Because your three patches are in, but you
> didn't arrange them to come after my 'clean up rwlock abuse' patch, as
> you seemed to be saying 'yes' to...
Doh, I misread your question, multiple times. I thought you were asking if I
wanted you to arrange your patches after this series.
Your series goes on top because I want to land this series in 6.9 to fix the
syzkaller splat (which was effectively introduced in 6.9), whereas your patch is
6.10 material.