From a0d46c1dd14c9645db19705fcb7e1a4e381bd180 Mon Sep 17 00:00:00 2001 From: Mostafa Saleh Date: Fri, 28 Apr 2023 12:54:02 +0000 Subject: [PATCH] ANDROID: KVM: arm64: iommu: Erase pvmfw from EL1 if possible Kernel IOMMU drivers can report system misconfiguration through pkvm_iommu_finalize(). Then EL2 can take the proper response, for example if there are missing IOMMUs, and DMA isolation can't be ensured, it would clear pvmfw so PVMs can't be launched. However, this is not clean as userspace can still query pvmfw info and launch PVMs that loops in undefined instruction aborts as pvmfw is cleared. To fix this, before deprivilege, the kernel will erase pvmfw if the IOMMUs are not finalised. Bug: 268607700 Test: Launch PVM with missing S2MPU => Fail immediately with -8 Test: Launch PVM with all S2MPU => Pass Change-Id: I9fd2440805f6b2f2ad4395ce61df5b272ed84fef Signed-off-by: Mostafa Saleh (cherry picked from commit 01ec18c52f65a3a784e236ce72aa983e65eca83c) --- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 5 ----- arch/arm64/kvm/hyp/nvhe/iommu.c | 10 +++------- arch/arm64/kvm/hyp/nvhe/pkvm.c | 11 ----------- arch/arm64/kvm/iommu.c | 13 +++++++++++-- arch/arm64/kvm/pkvm.c | 7 +++++++ 6 files changed, 23 insertions(+), 25 deletions(-) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 52a1901b38f7..740a62f88a10 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -406,6 +406,8 @@ int pkvm_iommu_resume(struct device *dev); */ int pkvm_iommu_finalize(int err); +bool pkvm_iommu_finalized(void); + struct vcpu_reset_state { unsigned long pc; unsigned long r0; diff --git a/arch/arm64/kvm/hyp/include/nvhe/pkvm.h b/arch/arm64/kvm/hyp/include/nvhe/pkvm.h index 9eba06503704..b11fd818fff9 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/pkvm.h +++ b/arch/arm64/kvm/hyp/include/nvhe/pkvm.h @@ -162,9 +162,4 @@ int pkvm_load_pvmfw_pages(struct pkvm_hyp_vm *vm, u64 ipa, phys_addr_t phys, u64 size); void pkvm_poison_pvmfw_pages(void); -/* - * Notify pKVM about events that can undermine pKVM security. - */ -void pkvm_handle_system_misconfiguration(enum pkvm_system_misconfiguration event); - #endif /* __ARM64_KVM_NVHE_PKVM_H__ */ diff --git a/arch/arm64/kvm/hyp/nvhe/iommu.c b/arch/arm64/kvm/hyp/nvhe/iommu.c index 98ef2e23d19f..4c56afee5e2c 100644 --- a/arch/arm64/kvm/hyp/nvhe/iommu.c +++ b/arch/arm64/kvm/hyp/nvhe/iommu.c @@ -456,6 +456,9 @@ int __pkvm_iommu_finalize(int err) { int ret = 0; + /* Err is not currently used in EL2.*/ + WARN_ON(err); + hyp_spin_lock(&iommu_registration_lock); if (!iommu_finalized) iommu_finalized = true; @@ -463,13 +466,6 @@ int __pkvm_iommu_finalize(int err) ret = -EPERM; hyp_spin_unlock(&iommu_registration_lock); - /* - * If finalize failed in EL1 driver for any reason, this means we can't trust the DMA - * isolation. So we have to inform pKVM to properly protect itself. - */ - if (!ret && err) - pkvm_handle_system_misconfiguration(NO_DMA_ISOLATION); - return ret; } diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c index a6cc23d91d81..bb25de654934 100644 --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c @@ -1570,14 +1570,3 @@ bool kvm_hyp_handle_hvc64(struct kvm_vcpu *vcpu, u64 *exit_code) return false; } - -/* - * Notify pKVM about events that can undermine pKVM security. - */ -void pkvm_handle_system_misconfiguration(enum pkvm_system_misconfiguration event) -{ - if (event == NO_DMA_ISOLATION) - pkvm_poison_pvmfw_pages(); - else - BUG(); -} diff --git a/arch/arm64/kvm/iommu.c b/arch/arm64/kvm/iommu.c index 0969a808d89d..006aeaccc3c9 100644 --- a/arch/arm64/kvm/iommu.c +++ b/arch/arm64/kvm/iommu.c @@ -6,6 +6,9 @@ #include +/* Did all IOMMUs register as expected. */ +static bool finalised; + static unsigned long dev_to_id(struct device *dev) { /* Use the struct device pointer as a unique identifier. */ @@ -59,6 +62,12 @@ EXPORT_SYMBOL(pkvm_iommu_resume); int pkvm_iommu_finalize(int err) { - return kvm_call_hyp_nvhe(__pkvm_iommu_finalize, err); + finalised = !err; + return kvm_call_hyp_nvhe(__pkvm_iommu_finalize, 0); +} +EXPORT_SYMBOL_GPL(pkvm_iommu_finalize); + +bool pkvm_iommu_finalized(void) +{ + return finalised; } -EXPORT_SYMBOL(pkvm_iommu_finalize); diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c index a87128975eae..b601ff98e9f2 100644 --- a/arch/arm64/kvm/pkvm.c +++ b/arch/arm64/kvm/pkvm.c @@ -448,6 +448,9 @@ static int __init pkvm_firmware_rmem_clear(void) return -EINVAL; memset(addr, 0, size); + /* Clear so user space doesn't get stale info via IOCTL. */ + pkvm_firmware_mem = NULL; + dcache_clean_poc((unsigned long)addr, (unsigned long)addr + size); memunmap(addr); return 0; @@ -501,6 +504,10 @@ static int __init finalize_pkvm(void) if (pkvm_load_early_modules()) pkvm_firmware_rmem_clear(); + /* If no DMA protection. */ + if (!pkvm_iommu_finalized()) + pkvm_firmware_rmem_clear(); + /* * Exclude HYP sections from kmemleak so that they don't get peeked * at, which would end badly once inaccessible.