From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4C019C433FE for ; Tue, 8 Dec 2020 15:57:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1200223AC0 for ; Tue, 8 Dec 2020 15:57:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730256AbgLHP5Z (ORCPT ); Tue, 8 Dec 2020 10:57:25 -0500 Received: from mail.kernel.org ([198.145.29.99]:49832 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729558AbgLHP5Y (ORCPT ); Tue, 8 Dec 2020 10:57:24 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5FDF823AA9; Tue, 8 Dec 2020 15:56:42 +0000 (UTC) Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94) (envelope-from ) id 1kmfM0-00H7fO-1u; Tue, 08 Dec 2020 15:56:40 +0000 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 08 Dec 2020 15:56:39 +0000 From: Marc Zyngier To: David Brazdil Cc: kvmarm@lists.cs.columbia.edu, James Morse , Julien Thierry , Suzuki K Poulose , Catalin Marinas , Will Deacon , Mark Rutland , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208142452.87237-2-dbrazdil@google.com> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> User-Agent: Roundcube Webmail/1.4.9 Message-ID: X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2020-12-08 14:24, David Brazdil wrote: > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > configured in the DT. However, the struct does not convey the > information whether these were set from DT or contain the default value > zero. This could be a problem for PSCI proxy in KVM protected mode. > > Extend config passed to KVM with a bit mask with individual bits set > depending on whether the corresponding function pointer in psci_ops is > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > Previously config was split into multiple global variables. Put > everything into a single struct for convenience. > > Reported-by: Mark Rutland > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > arch/arm64/kvm/arm.c | 14 +++++--- > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > 3 files changed, 70 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index 11beda85ee7e..828d50d40dc2 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -240,6 +241,25 @@ struct kvm_host_data { > struct kvm_pmu_events pmu_events; > }; > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > + > +struct kvm_host_psci_config { > + /* PSCI version used by host. */ > + u32 version; > + > + /* Function IDs used by host if version is v0.1. */ > + struct psci_0_1_function_ids function_ids_0_1; > + > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. > */ > + unsigned int enabled_functions_0_1; Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"? > +}; > + > +extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); > +#define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config) > + > struct vcpu_reset_state { > unsigned long pc; > unsigned long r0; > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 6e637d2b4cfb..6a2f4e01b04f 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -66,8 +66,6 @@ static DEFINE_PER_CPU(unsigned char, > kvm_arm_hardware_enabled); > DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS]; > -extern u32 kvm_nvhe_sym(kvm_host_psci_version); > -extern struct psci_0_1_function_ids > kvm_nvhe_sym(kvm_host_psci_0_1_function_ids); > > int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu) > { > @@ -1618,8 +1616,16 @@ static bool init_psci_relay(void) > return false; > } > > - kvm_nvhe_sym(kvm_host_psci_version) = psci_ops.get_version(); > - kvm_nvhe_sym(kvm_host_psci_0_1_function_ids) = > get_psci_0_1_function_ids(); > + kvm_host_psci_config.version = psci_ops.get_version(); > + > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > + kvm_host_psci_config.enabled_functions_0_1 = > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > + } > return true; > } > > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > index 08dc9de69314..0d6f4aa39621 100644 > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > @@ -22,9 +22,8 @@ void kvm_hyp_cpu_resume(unsigned long r0); > void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt); > > /* Config options set by the host. */ > -__ro_after_init u32 kvm_host_psci_version; > -__ro_after_init struct psci_0_1_function_ids > kvm_host_psci_0_1_function_ids; > -__ro_after_init s64 hyp_physvirt_offset; > +struct kvm_host_psci_config __ro_after_init kvm_host_psci_config; > +s64 __ro_after_init hyp_physvirt_offset; Unrelated change? > > #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset) > > @@ -54,12 +53,41 @@ static u64 get_psci_func_id(struct kvm_cpu_context > *host_ctxt) > return func_id; > } > > +static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) Don't bother with "inline" outside of an include file. It really doesn't mean much (the compiler is free to ignore it), and it is likely that the compiler will optimise better without guidance (not to mention this is hardly a fast path anyway). > +{ > + return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; > +} > + > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > + (func_id == > kvm_host_psci_config.function_ids_0_1.cpu_suspend); > +} > + > +static inline bool is_psci_0_1_cpu_on(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); > +} > + > +static inline bool is_psci_0_1_cpu_off(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); > +} > + > +static inline bool is_psci_0_1_migrate(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && > + (func_id == kvm_host_psci_config.function_ids_0_1.migrate); > +} > + > static bool is_psci_0_1_call(u64 func_id) > { > - return (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_on) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate); > + return is_psci_0_1_cpu_suspend(func_id) || > + is_psci_0_1_cpu_on(func_id) || > + is_psci_0_1_cpu_off(func_id) || > + is_psci_0_1_migrate(func_id); > } > > static bool is_psci_0_2_call(u64 func_id) > @@ -71,7 +99,7 @@ static bool is_psci_0_2_call(u64 func_id) > > static bool is_psci_call(u64 func_id) > { > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > return is_psci_0_1_call(func_id); > default: > @@ -248,12 +276,11 @@ asmlinkage void __noreturn > kvm_host_psci_cpu_entry(bool is_cpu_on) > > static unsigned long psci_0_1_handler(u64 func_id, struct > kvm_cpu_context *host_ctxt) > { > - if ((func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate)) > + if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) > return psci_forward(host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_on) > + else if (is_psci_0_1_cpu_on(func_id)) > return psci_cpu_on(func_id, host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) > + else if (is_psci_0_1_cpu_suspend(func_id)) > return psci_cpu_suspend(func_id, host_ctxt); > else > return PSCI_RET_NOT_SUPPORTED; > @@ -304,7 +331,7 @@ bool kvm_host_psci_handler(struct kvm_cpu_context > *host_ctxt) > if (!is_psci_call(func_id)) > return false; > > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > ret = psci_0_1_handler(func_id, host_ctxt); > break; Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues. Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06267C433FE for ; Tue, 8 Dec 2020 15:56:49 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 4DADC23AC0 for ; Tue, 8 Dec 2020 15:56:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4DADC23AC0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id B87574B259; Tue, 8 Dec 2020 10:56:47 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ym+fwwae72yM; Tue, 8 Dec 2020 10:56:46 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 2E3B64B124; Tue, 8 Dec 2020 10:56:46 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 041E94B102 for ; Tue, 8 Dec 2020 10:56:45 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1yFd4MTQOE6v for ; Tue, 8 Dec 2020 10:56:43 -0500 (EST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 93CD44B0E7 for ; Tue, 8 Dec 2020 10:56:43 -0500 (EST) Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5FDF823AA9; Tue, 8 Dec 2020 15:56:42 +0000 (UTC) Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94) (envelope-from ) id 1kmfM0-00H7fO-1u; Tue, 08 Dec 2020 15:56:40 +0000 MIME-Version: 1.0 Date: Tue, 08 Dec 2020 15:56:39 +0000 From: Marc Zyngier To: David Brazdil Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208142452.87237-2-dbrazdil@google.com> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> User-Agent: Roundcube Webmail/1.4.9 Message-ID: X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: kernel-team@android.com, Catalin Marinas , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon , kvmarm@lists.cs.columbia.edu X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On 2020-12-08 14:24, David Brazdil wrote: > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > configured in the DT. However, the struct does not convey the > information whether these were set from DT or contain the default value > zero. This could be a problem for PSCI proxy in KVM protected mode. > > Extend config passed to KVM with a bit mask with individual bits set > depending on whether the corresponding function pointer in psci_ops is > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > Previously config was split into multiple global variables. Put > everything into a single struct for convenience. > > Reported-by: Mark Rutland > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > arch/arm64/kvm/arm.c | 14 +++++--- > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > 3 files changed, 70 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index 11beda85ee7e..828d50d40dc2 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -240,6 +241,25 @@ struct kvm_host_data { > struct kvm_pmu_events pmu_events; > }; > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > + > +struct kvm_host_psci_config { > + /* PSCI version used by host. */ > + u32 version; > + > + /* Function IDs used by host if version is v0.1. */ > + struct psci_0_1_function_ids function_ids_0_1; > + > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. > */ > + unsigned int enabled_functions_0_1; Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"? > +}; > + > +extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); > +#define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config) > + > struct vcpu_reset_state { > unsigned long pc; > unsigned long r0; > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 6e637d2b4cfb..6a2f4e01b04f 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -66,8 +66,6 @@ static DEFINE_PER_CPU(unsigned char, > kvm_arm_hardware_enabled); > DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS]; > -extern u32 kvm_nvhe_sym(kvm_host_psci_version); > -extern struct psci_0_1_function_ids > kvm_nvhe_sym(kvm_host_psci_0_1_function_ids); > > int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu) > { > @@ -1618,8 +1616,16 @@ static bool init_psci_relay(void) > return false; > } > > - kvm_nvhe_sym(kvm_host_psci_version) = psci_ops.get_version(); > - kvm_nvhe_sym(kvm_host_psci_0_1_function_ids) = > get_psci_0_1_function_ids(); > + kvm_host_psci_config.version = psci_ops.get_version(); > + > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > + kvm_host_psci_config.enabled_functions_0_1 = > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > + } > return true; > } > > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > index 08dc9de69314..0d6f4aa39621 100644 > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > @@ -22,9 +22,8 @@ void kvm_hyp_cpu_resume(unsigned long r0); > void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt); > > /* Config options set by the host. */ > -__ro_after_init u32 kvm_host_psci_version; > -__ro_after_init struct psci_0_1_function_ids > kvm_host_psci_0_1_function_ids; > -__ro_after_init s64 hyp_physvirt_offset; > +struct kvm_host_psci_config __ro_after_init kvm_host_psci_config; > +s64 __ro_after_init hyp_physvirt_offset; Unrelated change? > > #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset) > > @@ -54,12 +53,41 @@ static u64 get_psci_func_id(struct kvm_cpu_context > *host_ctxt) > return func_id; > } > > +static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) Don't bother with "inline" outside of an include file. It really doesn't mean much (the compiler is free to ignore it), and it is likely that the compiler will optimise better without guidance (not to mention this is hardly a fast path anyway). > +{ > + return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; > +} > + > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > + (func_id == > kvm_host_psci_config.function_ids_0_1.cpu_suspend); > +} > + > +static inline bool is_psci_0_1_cpu_on(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); > +} > + > +static inline bool is_psci_0_1_cpu_off(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); > +} > + > +static inline bool is_psci_0_1_migrate(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && > + (func_id == kvm_host_psci_config.function_ids_0_1.migrate); > +} > + > static bool is_psci_0_1_call(u64 func_id) > { > - return (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_on) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate); > + return is_psci_0_1_cpu_suspend(func_id) || > + is_psci_0_1_cpu_on(func_id) || > + is_psci_0_1_cpu_off(func_id) || > + is_psci_0_1_migrate(func_id); > } > > static bool is_psci_0_2_call(u64 func_id) > @@ -71,7 +99,7 @@ static bool is_psci_0_2_call(u64 func_id) > > static bool is_psci_call(u64 func_id) > { > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > return is_psci_0_1_call(func_id); > default: > @@ -248,12 +276,11 @@ asmlinkage void __noreturn > kvm_host_psci_cpu_entry(bool is_cpu_on) > > static unsigned long psci_0_1_handler(u64 func_id, struct > kvm_cpu_context *host_ctxt) > { > - if ((func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate)) > + if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) > return psci_forward(host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_on) > + else if (is_psci_0_1_cpu_on(func_id)) > return psci_cpu_on(func_id, host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) > + else if (is_psci_0_1_cpu_suspend(func_id)) > return psci_cpu_suspend(func_id, host_ctxt); > else > return PSCI_RET_NOT_SUPPORTED; > @@ -304,7 +331,7 @@ bool kvm_host_psci_handler(struct kvm_cpu_context > *host_ctxt) > if (!is_psci_call(func_id)) > return false; > > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > ret = psci_0_1_handler(func_id, host_ctxt); > break; Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5DBC2C433FE for ; Tue, 8 Dec 2020 15:57:54 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 16D3223AC0 for ; Tue, 8 Dec 2020 15:57:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 16D3223AC0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=jQe45UHlxjraDGiEISp9o7zxC++RaeGUAJnvmiUNX0Y=; b=lPobsPpkqmimpc6xYRsy/Im5z 8mXbV7yto0Uxe0sKy3ULcuWNEZVDLRuCRJSoTRCGbhYvxb5XgMEPKqtHg5IaSYiEGok0mmfbILHTc W1g++J7sWEWF3RyQ9j5I5nKimiaMY5CVf4Ry7NMN+fmMM2Oaq8E0KSX8jlpPQ+ffPeU1uhurn+s/L kiQp869y7S+ztiuwuvYb/w5sRnqgHd94D1pM8nEZzJJHXapjYUWi67t9i1IU30HB4Oxj5LLaQzAOp 9mm0wA+NJmvkxS2jG0g0up/nkm/OO/oicoTfTFjXhXBIMtmovq3l1E4rHAMU0h0PTADcVlJJunEMS fnsvWGMWw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kmfM7-0002HN-2B; Tue, 08 Dec 2020 15:56:47 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kmfM3-0002GA-9v for linux-arm-kernel@lists.infradead.org; Tue, 08 Dec 2020 15:56:45 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5FDF823AA9; Tue, 8 Dec 2020 15:56:42 +0000 (UTC) Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94) (envelope-from ) id 1kmfM0-00H7fO-1u; Tue, 08 Dec 2020 15:56:40 +0000 MIME-Version: 1.0 Date: Tue, 08 Dec 2020 15:56:39 +0000 From: Marc Zyngier To: David Brazdil Subject: Re: [PATCH 1/6] kvm: arm64: Prevent use of invalid PSCI v0.1 function IDs In-Reply-To: <20201208142452.87237-2-dbrazdil@google.com> References: <20201208142452.87237-1-dbrazdil@google.com> <20201208142452.87237-2-dbrazdil@google.com> User-Agent: Roundcube Webmail/1.4.9 Message-ID: X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201208_105643_522511_587728B2 X-CRM114-Status: GOOD ( 29.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , kernel-team@android.com, Suzuki K Poulose , Catalin Marinas , linux-kernel@vger.kernel.org, James Morse , linux-arm-kernel@lists.infradead.org, Will Deacon , kvmarm@lists.cs.columbia.edu, Julien Thierry Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2020-12-08 14:24, David Brazdil wrote: > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > configured in the DT. However, the struct does not convey the > information whether these were set from DT or contain the default value > zero. This could be a problem for PSCI proxy in KVM protected mode. > > Extend config passed to KVM with a bit mask with individual bits set > depending on whether the corresponding function pointer in psci_ops is > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > Previously config was split into multiple global variables. Put > everything into a single struct for convenience. > > Reported-by: Mark Rutland > Signed-off-by: David Brazdil > --- > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > arch/arm64/kvm/arm.c | 14 +++++--- > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > 3 files changed, 70 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index 11beda85ee7e..828d50d40dc2 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -240,6 +241,25 @@ struct kvm_host_data { > struct kvm_pmu_events pmu_events; > }; > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > + > +struct kvm_host_psci_config { > + /* PSCI version used by host. */ > + u32 version; > + > + /* Function IDs used by host if version is v0.1. */ > + struct psci_0_1_function_ids function_ids_0_1; > + > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. > */ > + unsigned int enabled_functions_0_1; Nit: the conventional type for bitmaps is 'unsigned long'. Also, "enabled" seems odd. Isn't it actually "available"? > +}; > + > +extern struct kvm_host_psci_config kvm_nvhe_sym(kvm_host_psci_config); > +#define kvm_host_psci_config CHOOSE_NVHE_SYM(kvm_host_psci_config) > + > struct vcpu_reset_state { > unsigned long pc; > unsigned long r0; > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 6e637d2b4cfb..6a2f4e01b04f 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -66,8 +66,6 @@ static DEFINE_PER_CPU(unsigned char, > kvm_arm_hardware_enabled); > DEFINE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS]; > -extern u32 kvm_nvhe_sym(kvm_host_psci_version); > -extern struct psci_0_1_function_ids > kvm_nvhe_sym(kvm_host_psci_0_1_function_ids); > > int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu) > { > @@ -1618,8 +1616,16 @@ static bool init_psci_relay(void) > return false; > } > > - kvm_nvhe_sym(kvm_host_psci_version) = psci_ops.get_version(); > - kvm_nvhe_sym(kvm_host_psci_0_1_function_ids) = > get_psci_0_1_function_ids(); > + kvm_host_psci_config.version = psci_ops.get_version(); > + > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > + kvm_host_psci_config.function_ids_0_1 = get_psci_0_1_function_ids(); > + kvm_host_psci_config.enabled_functions_0_1 = > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : 0) | > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); > + } > return true; > } > > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > index 08dc9de69314..0d6f4aa39621 100644 > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > @@ -22,9 +22,8 @@ void kvm_hyp_cpu_resume(unsigned long r0); > void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt); > > /* Config options set by the host. */ > -__ro_after_init u32 kvm_host_psci_version; > -__ro_after_init struct psci_0_1_function_ids > kvm_host_psci_0_1_function_ids; > -__ro_after_init s64 hyp_physvirt_offset; > +struct kvm_host_psci_config __ro_after_init kvm_host_psci_config; > +s64 __ro_after_init hyp_physvirt_offset; Unrelated change? > > #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset) > > @@ -54,12 +53,41 @@ static u64 get_psci_func_id(struct kvm_cpu_context > *host_ctxt) > return func_id; > } > > +static inline bool is_psci_0_1_function_enabled(unsigned int fn_bit) Don't bother with "inline" outside of an include file. It really doesn't mean much (the compiler is free to ignore it), and it is likely that the compiler will optimise better without guidance (not to mention this is hardly a fast path anyway). > +{ > + return kvm_host_psci_config.enabled_functions_0_1 & fn_bit; > +} > + > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > + (func_id == > kvm_host_psci_config.function_ids_0_1.cpu_suspend); > +} > + > +static inline bool is_psci_0_1_cpu_on(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_ON) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_on); > +} > + > +static inline bool is_psci_0_1_cpu_off(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_OFF) && > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_off); > +} > + > +static inline bool is_psci_0_1_migrate(u64 func_id) > +{ > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_MIGRATE) && > + (func_id == kvm_host_psci_config.function_ids_0_1.migrate); > +} > + > static bool is_psci_0_1_call(u64 func_id) > { > - return (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_on) || > - (func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate); > + return is_psci_0_1_cpu_suspend(func_id) || > + is_psci_0_1_cpu_on(func_id) || > + is_psci_0_1_cpu_off(func_id) || > + is_psci_0_1_migrate(func_id); > } > > static bool is_psci_0_2_call(u64 func_id) > @@ -71,7 +99,7 @@ static bool is_psci_0_2_call(u64 func_id) > > static bool is_psci_call(u64 func_id) > { > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > return is_psci_0_1_call(func_id); > default: > @@ -248,12 +276,11 @@ asmlinkage void __noreturn > kvm_host_psci_cpu_entry(bool is_cpu_on) > > static unsigned long psci_0_1_handler(u64 func_id, struct > kvm_cpu_context *host_ctxt) > { > - if ((func_id == kvm_host_psci_0_1_function_ids.cpu_off) || > - (func_id == kvm_host_psci_0_1_function_ids.migrate)) > + if (is_psci_0_1_cpu_off(func_id) || is_psci_0_1_migrate(func_id)) > return psci_forward(host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_on) > + else if (is_psci_0_1_cpu_on(func_id)) > return psci_cpu_on(func_id, host_ctxt); > - else if (func_id == kvm_host_psci_0_1_function_ids.cpu_suspend) > + else if (is_psci_0_1_cpu_suspend(func_id)) > return psci_cpu_suspend(func_id, host_ctxt); > else > return PSCI_RET_NOT_SUPPORTED; > @@ -304,7 +331,7 @@ bool kvm_host_psci_handler(struct kvm_cpu_context > *host_ctxt) > if (!is_psci_call(func_id)) > return false; > > - switch (kvm_host_psci_version) { > + switch (kvm_host_psci_config.version) { > case PSCI_VERSION(0, 1): > ret = psci_0_1_handler(func_id, host_ctxt); > break; Otherwise looks OK. Don't bother respinning the series for my comments, I can tidy things up as I apply it if there are no other issues. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel