Skip to content

Commit

Permalink
KVM: Add helpers to consolidate gfn_to_pfn_cache's page split check
Browse files Browse the repository at this point in the history
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.

Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Link: https://lore.kernel.org/r/20240320001542.3203871-2-seanjc@google.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
  • Loading branch information
sean-jc committed Apr 8, 2024
1 parent fec50db commit 18f06e9
Showing 1 changed file with 19 additions and 8 deletions.
27 changes: 19 additions & 8 deletions virt/kvm/pfncache.c
Expand Up @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 18f06e9

Please sign in to comment.