| Message ID | 20220128121912.509006-38-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: ARMv8.3/8.4 Nested Virtualization support | expand |
Hi, On Fri, Jan 28, 2022 at 12:18:45PM +0000, Marc Zyngier wrote: > When mapping a page in a shadow stage-2, special care must be > taken not to be more permissive than the guest is (writable or > readable page when the guest hasn't set that permission). > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/kvm_nested.h | 15 +++++++++++++++ > arch/arm64/kvm/mmu.c | 14 +++++++++++++- > arch/arm64/kvm/nested.c | 2 +- > 3 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > index 4fad4d3848ce..f4b846d09d86 100644 > --- a/arch/arm64/include/asm/kvm_nested.h > +++ b/arch/arm64/include/asm/kvm_nested.h > @@ -97,6 +97,21 @@ static inline u32 kvm_s2_trans_esr(struct kvm_s2_trans *trans) > return trans->esr; > } > > +static inline bool kvm_s2_trans_readable(struct kvm_s2_trans *trans) > +{ > + return trans->readable; > +} > + > +static inline bool kvm_s2_trans_writable(struct kvm_s2_trans *trans) > +{ > + return trans->writable; > +} > + > +static inline bool kvm_s2_trans_executable(struct kvm_s2_trans *trans) > +{ > + return !(trans->upper_attr & BIT(54)); > +} > + > extern int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa, > struct kvm_s2_trans *result); > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 36f7ecb4f81b..7c56e1522d3c 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1247,6 +1247,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > if (exec_fault && device) > return -ENOEXEC; > > + /* > + * Potentially reduce shadow S2 permissions to match the guest's own > + * S2. For exec faults, we'd only reach this point if the guest > + * actually allowed it (see kvm_s2_handle_perm_fault). > + */ > + if (kvm_is_shadow_s2_fault(vcpu)) { > + writable &= kvm_s2_trans_writable(nested); I was a bit confused about writable being true when write_fault is false. After some digging, it turns out that hva_to_pfn() oportunistically makes writable true, even for read faults. > + if (!kvm_s2_trans_readable(nested)) > + prot &= ~KVM_PGTABLE_PROT_R; The local variable "prot" is initialized to KVM_PGTABLE_PROT_R, so this check makes sense. > + } > + > spin_lock(&kvm->mmu_lock); > pgt = vcpu->arch.hw_mmu->pgt; > if (mmu_notifier_retry(kvm, mmu_seq)) > @@ -1285,7 +1296,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > > if (device) > prot |= KVM_PGTABLE_PROT_DEVICE; > - else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) > + else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC) && > + kvm_s2_trans_executable(nested)) > prot |= KVM_PGTABLE_PROT_X; > > /* > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > index 0a9708f776fc..a74ffb1d2064 100644 > --- a/arch/arm64/kvm/nested.c > +++ b/arch/arm64/kvm/nested.c > @@ -481,7 +481,7 @@ int kvm_s2_handle_perm_fault(struct kvm_vcpu *vcpu, struct kvm_s2_trans *trans) > return 0; > > if (kvm_vcpu_trap_is_iabt(vcpu)) { > - forward_fault = (trans->upper_attr & BIT(54)); > + forward_fault = !kvm_s2_trans_executable(trans); > } else { > bool write_fault = kvm_is_write_fault(vcpu); The patch looks good to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex
diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h index 4fad4d3848ce..f4b846d09d86 100644 --- a/arch/arm64/include/asm/kvm_nested.h +++ b/arch/arm64/include/asm/kvm_nested.h @@ -97,6 +97,21 @@ static inline u32 kvm_s2_trans_esr(struct kvm_s2_trans *trans) return trans->esr; } +static inline bool kvm_s2_trans_readable(struct kvm_s2_trans *trans) +{ + return trans->readable; +} + +static inline bool kvm_s2_trans_writable(struct kvm_s2_trans *trans) +{ + return trans->writable; +} + +static inline bool kvm_s2_trans_executable(struct kvm_s2_trans *trans) +{ + return !(trans->upper_attr & BIT(54)); +} + extern int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa, struct kvm_s2_trans *result); diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 36f7ecb4f81b..7c56e1522d3c 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1247,6 +1247,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, if (exec_fault && device) return -ENOEXEC; + /* + * Potentially reduce shadow S2 permissions to match the guest's own + * S2. For exec faults, we'd only reach this point if the guest + * actually allowed it (see kvm_s2_handle_perm_fault). + */ + if (kvm_is_shadow_s2_fault(vcpu)) { + writable &= kvm_s2_trans_writable(nested); + if (!kvm_s2_trans_readable(nested)) + prot &= ~KVM_PGTABLE_PROT_R; + } + spin_lock(&kvm->mmu_lock); pgt = vcpu->arch.hw_mmu->pgt; if (mmu_notifier_retry(kvm, mmu_seq)) @@ -1285,7 +1296,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, if (device) prot |= KVM_PGTABLE_PROT_DEVICE; - else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) + else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC) && + kvm_s2_trans_executable(nested)) prot |= KVM_PGTABLE_PROT_X; /* diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c index 0a9708f776fc..a74ffb1d2064 100644 --- a/arch/arm64/kvm/nested.c +++ b/arch/arm64/kvm/nested.c @@ -481,7 +481,7 @@ int kvm_s2_handle_perm_fault(struct kvm_vcpu *vcpu, struct kvm_s2_trans *trans) return 0; if (kvm_vcpu_trap_is_iabt(vcpu)) { - forward_fault = (trans->upper_attr & BIT(54)); + forward_fault = !kvm_s2_trans_executable(trans); } else { bool write_fault = kvm_is_write_fault(vcpu);
When mapping a page in a shadow stage-2, special care must be taken not to be more permissive than the guest is (writable or readable page when the guest hasn't set that permission). Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_nested.h | 15 +++++++++++++++ arch/arm64/kvm/mmu.c | 14 +++++++++++++- arch/arm64/kvm/nested.c | 2 +- 3 files changed, 29 insertions(+), 2 deletions(-)