KVM: arm64: Prevent unconditional donation of unmapped regions from the host
Will Deacon
maintainerPATCH 1/1 KVM: arm64: Prevent unconditional donation of unmapped regions from the host
Since host stage-2 mappings are created lazily, we cannot rely solely on the pte in order to recover the target physical address when checking a host-initiated memory transition as this permits donation of unmapped regions corresponding to MMIO or "no-map" memory.
Instead of inspecting the pte, move the addr_is_allowed_memory() check into the host callback function where it is passed the physical address directly from the walker.
--- arch/arm64/kvm/hyp/nvhe/mem_protect.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
arch/arm64/kvm/hyp/nvhe/mem_protect.c +7 -7
@@ -575,7 +575,7 @@ struct pkvm_mem_donation {struct check_walk_data {enum pkvm_page_state desired; -enum pkvm_page_state (*get_page_state)(kvm_pte_t pte); +enum pkvm_page_state (*get_page_state)(kvm_pte_t pte,u64 addr); }; static int __check_page_state_visitor(const struct kvm_pgtable_visit_ctx *ctx,
@@ -583,10 +583,7 @@ static int __check_page_state_visitor(const struct kvm_pgtable_visit_ctx *ctx, {struct check_walk_data *d = ctx->arg; -if (kvm_pte_valid (ctx->old) && !addr_is_allowed_memory (kvm_pte_to_phys (ctx->old))) -return -EINVAL ; - -return d->get_page_state (ctx->old) == d->desired ?0 : -EPERM ; +return d->get_page_state (ctx->old, ctx->addr) == d->desired ?0 : -EPERM ; } static int check_page_state_range(struct kvm_pgtable *pgt, u64 addr, u64 size,
@@ -601,8 +598,11 @@ static int check_page_state_range(struct kvm_pgtable *pgt, u64 addr, u64 size,return kvm_pgtable_walk (pgt, addr, size, &walker); } -static enum pkvm_page_state host_get_page_state (kvm_pte_t pte) +static enum pkvm_page_state host_get_page_state (kvm_pte_t pte,u64 addr) { +if (!addr_is_allowed_memory (addr)) +return PKVM_NOPAGE ; +if (!kvm_pte_valid (pte) && pte)return PKVM_NOPAGE ;
@@ -709,7 +709,7 @@ static int host_complete_donation(u64 addr, const struct pkvm_mem_transition *txreturn host_stage2_set_owner_locked (addr, size, host_id); } -static enum pkvm_page_state hyp_get_page_state (kvm_pte_t pte) +static enum pkvm_page_state hyp_get_page_state (kvm_pte_t pte,u64 addr) {if (!kvm_pte_valid (pte))return PKVM_NOPAGE ;
Signature · 1 line
2.40.1.698.g37aff9b760-goog
Hi Will,
Quoted 8 lines · Will Deacon
Since host stage-2 mappings are created lazily, we cannot rely solely on the pte in order to recover the target physical address when checking a host-initiated memory transition as this permits donation of unmapped regions corresponding to MMIO or "no-map" memory.
Instead of inspecting the pte, move the
addr_is_allowed_memory()check into the host callback function where it is passed the physical address directly from the walker.
nit: the subtext here is that the host's stage-2 is an identity map. That part might not be entirely obvious, as you're passing through the input address of the walk.
Not worth respinning, but I wonder if some additional documentation on host stage-2 management would help folks outside of pKVM with reviewing pKVM changes. Cache behavior could also use a bit more clarification so folks understand why pKVM elides CMOs and doesn't use FWB.
Of course, I'm likely fooling myself in believing that additional documentation will get non-pKVM folks to review pKVM changes :)
Quoted 3 lines
Cc Quentin Perret <[email protected]>Fixes e82edcc75c4e ("KVM: arm64: Implement do_share() helper for sharing memory")Signed-off-by Will Deacon <[email protected]>
Quoted 54 lines
--- arch/arm64/kvm/hyp/nvhe/mem_protect.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)arch/arm64/kvm/hyp/nvhe/mem_protect.c +7
-7@@ -575,7 +575,7 @@ struct pkvm_mem_donation {struct check_walk_data {enum pkvm_page_state desired; -enum pkvm_page_state (*get_page_state)(kvm_pte_t pte); +enum pkvm_page_state (*get_page_state)(kvm_pte_t pte,u64 addr); }; static int __check_page_state_visitor(const struct kvm_pgtable_visit_ctx *ctx,@@ -583,10 +583,7 @@ static int __check_page_state_visitor(const struct kvm_pgtable_visit_ctx *ctx, {struct check_walk_data *d = ctx->arg; -if (kvm_pte_valid (ctx->old) && !addr_is_allowed_memory (kvm_pte_to_phys (ctx->old))) -return -EINVAL ; - -return d->get_page_state (ctx->old) == d->desired ?0 : -EPERM ; +return d->get_page_state (ctx->old, ctx->addr) == d->desired ?0 : -EPERM ; } static int check_page_state_range(struct kvm_pgtable *pgt, u64 addr, u64 size,@@ -601,8 +598,11 @@ static int check_page_state_range(struct kvm_pgtable *pgt, u64 addr, u64 size,return kvm_pgtable_walk (pgt, addr, size, &walker); } -static enum pkvm_page_state host_get_page_state (kvm_pte_t pte) +static enum pkvm_page_state host_get_page_state (kvm_pte_t pte,u64 addr) { +if (!addr_is_allowed_memory (addr)) +return PKVM_NOPAGE ; +if (!kvm_pte_valid (pte) && pte)return PKVM_NOPAGE ;@@ -709,7 +709,7 @@ static int host_complete_donation(u64 addr, const struct pkvm_mem_transition *txreturn host_stage2_set_owner_locked (addr, size, host_id); } -static enum pkvm_page_state hyp_get_page_state (kvm_pte_t pte) +static enum pkvm_page_state hyp_get_page_state (kvm_pte_t pte,u64 addr) {if (!kvm_pte_valid (pte))return PKVM_NOPAGE ;Signature · 1 line
2.40.1.698.g37aff9b760-goog
Signature · 2 lines
Thanks, Oliver
Quoted 8 lines · Will Deacon
Since host stage-2 mappings are created lazily, we cannot rely solely on the pte in order to recover the target physical address when checking a host-initiated memory transition as this permits donation of unmapped regions corresponding to MMIO or "no-map" memory.
Instead of inspecting the pte, move the
addr_is_allowed_memory()check into the host callback function where it is passed the physical address directly from the walker.
Applied to fixes, thanks!
[1/1] KVM: arm64: Prevent unconditional donation of unmapped regions from the host
commit: 09cce60bddd6461a93a5bf434265a47827d1bc6f
Cheers,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.