From cafb8d2a6238e43db326b79d508d186a906da2af Mon Sep 17 00:00:00 2001 From: Quentin Perret Date: Fri, 6 Jan 2023 16:49:53 +0000 Subject: [PATCH] ANDROID: KVM: arm64: Rework pKVM module locking Much of the functions related to pKVM modules at EL2 are currently guarded by the pkvm_modules_lock. This lock is only useful in case of module hypercalls racing with __pkvm_close_module_registration(). However, the latter is by definition a privileged operation, so we can trust that the host is not trying to exploit races to attack EL2 at this point. As such, let's remove the pkvm_modules_lock and re-use the existing concept of privileged hypercalls instead. To do so, the hypercall that limits the privileged range is moved dynamically depending on whether pKVM modules are supported or not. Bug: 357781595 Bug: 264070847 Change-Id: I6924471339f2123ab244cdb71ffcb2a299fa75a4 Signed-off-by: Quentin Perret --- arch/arm64/include/asm/kvm_asm.h | 17 +++--- arch/arm64/include/asm/kvm_hyp.h | 2 - arch/arm64/kvm/hyp/include/nvhe/modules.h | 8 +-- arch/arm64/kvm/hyp/nvhe/hyp-main.c | 31 ++++++++--- arch/arm64/kvm/hyp/nvhe/mm.c | 38 ++++++-------- arch/arm64/kvm/hyp/nvhe/modules.c | 64 ++--------------------- arch/arm64/kvm/pkvm.c | 8 ++- 7 files changed, 65 insertions(+), 103 deletions(-) diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index 6077f5b1f98b..029cfa257abd 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -66,6 +66,17 @@ enum __kvm_host_smccc_func { __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid, __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_range, __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context, + + /* + * __pkvm_alloc_module_va may temporarily serve as the privileged hcall + * limit when module loading is enabled, see early_pkvm_enable_modules(). + */ + __KVM_HOST_SMCCC_FUNC___pkvm_alloc_module_va, + __KVM_HOST_SMCCC_FUNC___pkvm_map_module_page, + __KVM_HOST_SMCCC_FUNC___pkvm_unmap_module_page, + __KVM_HOST_SMCCC_FUNC___pkvm_init_module, + __KVM_HOST_SMCCC_FUNC___pkvm_register_hcall, + __KVM_HOST_SMCCC_FUNC___pkvm_close_module_registration, __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize, /* Hypercalls available after pKVM finalisation */ @@ -93,12 +104,6 @@ enum __kvm_host_smccc_func { __KVM_HOST_SMCCC_FUNC___pkvm_swap_reader_tracing, __KVM_HOST_SMCCC_FUNC___pkvm_enable_event, __KVM_HOST_SMCCC_FUNC___pkvm_selftest_event, - __KVM_HOST_SMCCC_FUNC___pkvm_register_hcall, - __KVM_HOST_SMCCC_FUNC___pkvm_alloc_module_va, - __KVM_HOST_SMCCC_FUNC___pkvm_map_module_page, - __KVM_HOST_SMCCC_FUNC___pkvm_unmap_module_page, - __KVM_HOST_SMCCC_FUNC___pkvm_init_module, - __KVM_HOST_SMCCC_FUNC___pkvm_close_module_registration, /* * Start of the dynamically registered hypercalls. Start a bit diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h index 931183c7fe3a..aadb9a9500dc 100644 --- a/arch/arm64/include/asm/kvm_hyp.h +++ b/arch/arm64/include/asm/kvm_hyp.h @@ -148,6 +148,4 @@ extern unsigned long kvm_nvhe_sym(__icache_flags); extern unsigned int kvm_nvhe_sym(kvm_arm_vmid_bits); extern unsigned int kvm_nvhe_sym(kvm_host_sve_max_vl); -extern bool kvm_nvhe_sym(__pkvm_modules_enabled); - #endif /* __ARM64_KVM_HYP_H__ */ diff --git a/arch/arm64/kvm/hyp/include/nvhe/modules.h b/arch/arm64/kvm/hyp/include/nvhe/modules.h index 079e9b989705..e7a02720e586 100644 --- a/arch/arm64/kvm/hyp/include/nvhe/modules.h +++ b/arch/arm64/kvm/hyp/include/nvhe/modules.h @@ -11,13 +11,12 @@ int __pkvm_register_hyp_panic_notifier(void (*cb)(struct user_pt_regs *)); enum pkvm_psci_notification; int __pkvm_register_psci_notifier(void (*cb)(enum pkvm_psci_notification, struct user_pt_regs *)); +int reset_pkvm_priv_hcall_limit(void); + #ifdef CONFIG_MODULES int __pkvm_init_module(void *module_init); int __pkvm_register_hcall(unsigned long hfn_hyp_va); int handle_host_dynamic_hcall(struct kvm_cpu_context *host_ctxt); -void pkvm_modules_lock(void); -void pkvm_modules_unlock(void); -bool pkvm_modules_enabled(void); int __pkvm_close_module_registration(void); #else static inline int __pkvm_init_module(void *module_init) { return -EOPNOTSUPP; } @@ -27,8 +26,5 @@ static inline int handle_host_dynamic_hcall(struct kvm_cpu_context *host_ctxt) { return HCALL_UNHANDLED; } -static inline void pkvm_modules_lock(void) { } -static inline void pkvm_modules_unlock(void) { } -static inline bool pkvm_modules_enabled(void) { return false; } static inline int __pkvm_close_module_registration(void) { return -EOPNOTSUPP; } #endif diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index 2aa132bdbef7..b8d50e706894 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -1361,6 +1361,13 @@ static const hcall_t host_hcall[] = { HANDLE_FUNC(__kvm_tlb_flush_vmid), HANDLE_FUNC(__kvm_tlb_flush_vmid_range), HANDLE_FUNC(__kvm_flush_cpu_context), + + HANDLE_FUNC(__pkvm_alloc_module_va), + HANDLE_FUNC(__pkvm_map_module_page), + HANDLE_FUNC(__pkvm_unmap_module_page), + HANDLE_FUNC(__pkvm_init_module), + HANDLE_FUNC(__pkvm_register_hcall), + HANDLE_FUNC(__pkvm_close_module_registration), HANDLE_FUNC(__pkvm_prot_finalize), HANDLE_FUNC(__pkvm_host_share_hyp), @@ -1387,14 +1394,24 @@ static const hcall_t host_hcall[] = { HANDLE_FUNC(__pkvm_swap_reader_tracing), HANDLE_FUNC(__pkvm_enable_event), HANDLE_FUNC(__pkvm_selftest_event), - HANDLE_FUNC(__pkvm_alloc_module_va), - HANDLE_FUNC(__pkvm_map_module_page), - HANDLE_FUNC(__pkvm_unmap_module_page), - HANDLE_FUNC(__pkvm_init_module), - HANDLE_FUNC(__pkvm_register_hcall), - HANDLE_FUNC(__pkvm_close_module_registration), }; +unsigned long pkvm_priv_hcall_limit __ro_after_init = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize; + +int reset_pkvm_priv_hcall_limit(void) +{ + unsigned long *addr; + + if (pkvm_priv_hcall_limit == __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize) + return -EACCES; + + addr = hyp_fixmap_map(__hyp_pa(&pkvm_priv_hcall_limit)); + *addr = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize; + hyp_fixmap_unmap(); + + return 0; +} + static void handle_host_hcall(struct kvm_cpu_context *host_ctxt) { DECLARE_REG(unsigned long, id, host_ctxt, 0); @@ -1414,7 +1431,7 @@ static void handle_host_hcall(struct kvm_cpu_context *host_ctxt) * returns -EPERM after the first call for a given CPU. */ if (static_branch_unlikely(&kvm_protected_mode_initialized)) - hcall_min = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize; + hcall_min = pkvm_priv_hcall_limit; id &= ~ARM_SMCCC_CALL_HINTS; id -= KVM_HOST_SMCCC_ID(0); diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c index 9105b47b847c..8f809acbba52 100644 --- a/arch/arm64/kvm/hyp/nvhe/mm.c +++ b/arch/arm64/kvm/hyp/nvhe/mm.c @@ -114,11 +114,14 @@ int __pkvm_create_private_mapping(phys_addr_t phys, size_t size, #ifdef CONFIG_NVHE_EL2_DEBUG static unsigned long mod_range_start = ULONG_MAX; static unsigned long mod_range_end; +static DEFINE_HYP_SPINLOCK(mod_range_lock); static void update_mod_range(unsigned long addr, size_t size) { + hyp_spin_lock(&mod_range_lock); mod_range_start = min(mod_range_start, addr); mod_range_end = max(mod_range_end, addr + size); + hyp_spin_unlock(&mod_range_lock); } static void assert_in_mod_range(unsigned long addr) @@ -128,7 +131,9 @@ static void assert_in_mod_range(unsigned long addr) * allocations between modules being loaded, but in practice that is * probably going to be allocation initiated by the modules themselves. */ + hyp_spin_lock(&mod_range_lock); WARN_ON(addr < mod_range_start || mod_range_end <= addr); + hyp_spin_unlock(&mod_range_lock); } #else static inline void update_mod_range(unsigned long addr, size_t size) { } @@ -140,12 +145,8 @@ void *__pkvm_alloc_module_va(u64 nr_pages) size_t size = nr_pages << PAGE_SHIFT; unsigned long addr = 0; - pkvm_modules_lock(); - if (pkvm_modules_enabled()) { - if (!pkvm_alloc_private_va_range(size, &addr)) - update_mod_range(addr, size); - } - pkvm_modules_unlock(); + if (!pkvm_alloc_private_va_range(size, &addr)) + update_mod_range(addr, size); return (void *)addr; } @@ -153,34 +154,27 @@ void *__pkvm_alloc_module_va(u64 nr_pages) int __pkvm_map_module_page(u64 pfn, void *va, enum kvm_pgtable_prot prot) { unsigned long addr = (unsigned long)va; - int ret = -EACCES; - - pkvm_modules_lock(); + int ret; assert_in_mod_range(addr); - if (!pkvm_modules_enabled()) - goto err; - ret = __pkvm_host_donate_hyp(pfn, 1); if (ret) - goto err; + return ret; ret = __pkvm_create_mappings(addr, PAGE_SIZE, hyp_pfn_to_phys(pfn), prot); -err: - pkvm_modules_unlock(); + if (ret) { + WARN_ON(__pkvm_hyp_donate_host(pfn, 1)); + return ret; + } - return ret; + return 0; } void __pkvm_unmap_module_page(u64 pfn, void *va) { - pkvm_modules_lock(); - if (pkvm_modules_enabled()) { - WARN_ON(__pkvm_hyp_donate_host(pfn, 1)); - pkvm_remove_mappings(va, va + PAGE_SIZE); - } - pkvm_modules_unlock(); + WARN_ON(__pkvm_hyp_donate_host(pfn, 1)); + pkvm_remove_mappings(va, va + PAGE_SIZE); } int pkvm_create_mappings_locked(void *from, void *to, enum kvm_pgtable_prot prot) diff --git a/arch/arm64/kvm/hyp/nvhe/modules.c b/arch/arm64/kvm/hyp/nvhe/modules.c index 877e832680e3..1119f435adbc 100644 --- a/arch/arm64/kvm/hyp/nvhe/modules.c +++ b/arch/arm64/kvm/hyp/nvhe/modules.c @@ -17,25 +17,6 @@ static void __kvm_flush_dcache_to_poc(void *addr, size_t size) kvm_flush_dcache_to_poc((unsigned long)addr, (unsigned long)size); } -DEFINE_HYP_SPINLOCK(modules_lock); -bool __pkvm_modules_enabled __ro_after_init; - -void pkvm_modules_lock(void) -{ - hyp_spin_lock(&modules_lock); -} - -void pkvm_modules_unlock(void) -{ - hyp_spin_unlock(&modules_lock); -} - -bool pkvm_modules_enabled(void) -{ - return __pkvm_modules_enabled; -} - - static void __update_hcr_el2(unsigned long set_mask, unsigned long clear_mask) { struct kvm_nvhe_init_params *params = this_cpu_ptr(&kvm_init_params); @@ -56,7 +37,7 @@ static void __update_hfgwtr_el2(unsigned long set_mask, unsigned long clear_mask write_sysreg_s(params->hfgwtr_el2, SYS_HFGWTR_EL2); } -static u64 early_lm_pages; +static atomic_t early_lm_pages; static void *__pkvm_linear_map_early(phys_addr_t phys, size_t size, enum kvm_pgtable_prot prot) { void *addr = NULL; @@ -65,18 +46,12 @@ static void *__pkvm_linear_map_early(phys_addr_t phys, size_t size, enum kvm_pgt if (!PAGE_ALIGNED(phys) || !PAGE_ALIGNED(size)) return NULL; - pkvm_modules_lock(); - if (!__pkvm_modules_enabled) - goto out; - addr = __hyp_va(phys); ret = pkvm_create_mappings(addr, addr + size, prot); if (ret) addr = NULL; else - early_lm_pages += size >> PAGE_SHIFT; -out: - pkvm_modules_unlock(); + atomic_add(size, &early_lm_pages); return addr; } @@ -89,26 +64,15 @@ static void __pkvm_linear_unmap_early(void *addr, size_t size) int __pkvm_close_module_registration(void) { - int ret; - - pkvm_modules_lock(); /* * Page ownership tracking might go out of sync if there are stale * entries in pKVM's linear map range, so they must really be gone by * now. */ - WARN_ON(early_lm_pages); - - ret = __pkvm_modules_enabled ? 0 : -EACCES; - if (!ret) { - void *addr = hyp_fixmap_map(__hyp_pa(&__pkvm_modules_enabled)); - *(bool *)addr = false; - hyp_fixmap_unmap(); - } - pkvm_modules_unlock(); + WARN_ON(atomic_read(&early_lm_pages)); + return reset_pkvm_priv_hcall_limit(); /* The fuse is blown! No way back until reset */ - return ret; } static int __pkvm_module_host_donate_hyp(u64 pfn, u64 nr_pages) @@ -155,18 +119,8 @@ const struct pkvm_module_ops module_ops = { int __pkvm_init_module(void *module_init) { int (*do_module_init)(const struct pkvm_module_ops *ops) = module_init; - int ret; - pkvm_modules_lock(); - if (!pkvm_modules_enabled()) { - ret = -EACCES; - goto err; - } - ret = do_module_init(&module_ops); -err: - pkvm_modules_unlock(); - - return ret; + return do_module_init(&module_ops); } #define MAX_DYNAMIC_HCALLS 128 @@ -215,12 +169,6 @@ int __pkvm_register_hcall(unsigned long hvn_hyp_va) dyn_hcall_t hfn = (void *)hvn_hyp_va; int reserved_id, ret; - pkvm_modules_lock(); - if (!pkvm_modules_enabled()) { - ret = -EACCES; - goto err; - } - hyp_spin_lock(&dyn_hcall_lock); reserved_id = atomic_read(&num_dynamic_hcalls); @@ -241,8 +189,6 @@ int __pkvm_register_hcall(unsigned long hvn_hyp_va) ret = reserved_id + __KVM_HOST_SMCCC_FUNC___dynamic_hcalls; err_hcall_unlock: hyp_spin_unlock(&dyn_hcall_lock); -err: - pkvm_modules_unlock(); return ret; }; diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c index 05e823d3b9fe..c1b0c33f77ba 100644 --- a/arch/arm64/kvm/pkvm.c +++ b/arch/arm64/kvm/pkvm.c @@ -567,7 +567,13 @@ int pkvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap) #ifdef CONFIG_MODULES static int __init early_pkvm_enable_modules(char *arg) { - kvm_nvhe_sym(__pkvm_modules_enabled) = true; + extern unsigned long kvm_nvhe_sym(pkvm_priv_hcall_limit); + + /* + * Move the limit to allow module loading HVCs. It will be moved back to + * its original position in __pkvm_close_module_registration(). + */ + kvm_nvhe_sym(pkvm_priv_hcall_limit) = __KVM_HOST_SMCCC_FUNC___pkvm_alloc_module_va; return 0; }