diff mbox series

[5/5] KVM: arm64: Disable privileged hypercalls after pKVM finalisation

Message ID 20210923112256.15767-6-will@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Restrict host hypercalls when pKVM is enabled | expand

Commit Message

Will Deacon Sept. 23, 2021, 11:22 a.m. UTC
After pKVM has been 'finalised' using the __pkvm_prot_finalize hypercall,
the calling CPU will have a Stage-2 translation enabled to prevent access
to memory pages owned by EL2.

Although this forms a significant part of the process to deprivilege the
host kernel, we also need to ensure that the hypercall interface is
reduced so that the EL2 code cannot, for example, be re-initialised using
a new set of vectors.

Re-order the hypercalls so that only a suffix remains available after
finalisation of pKVM.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_asm.h   | 43 ++++++++++++++++--------------
 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 26 +++++++++++-------
 2 files changed, 39 insertions(+), 30 deletions(-)

Comments

Marc Zyngier Sept. 23, 2021, 12:56 p.m. UTC | #1
On Thu, 23 Sep 2021 12:22:56 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> After pKVM has been 'finalised' using the __pkvm_prot_finalize hypercall,
> the calling CPU will have a Stage-2 translation enabled to prevent access
> to memory pages owned by EL2.
> 
> Although this forms a significant part of the process to deprivilege the
> host kernel, we also need to ensure that the hypercall interface is
> reduced so that the EL2 code cannot, for example, be re-initialised using
> a new set of vectors.
> 
> Re-order the hypercalls so that only a suffix remains available after
> finalisation of pKVM.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_asm.h   | 43 ++++++++++++++++--------------
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 26 +++++++++++-------
>  2 files changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index e86045ac43ba..68630fd382c5 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -43,27 +43,30 @@
>  
>  #define KVM_HOST_SMCCC_FUNC(name) KVM_HOST_SMCCC_ID(__KVM_HOST_SMCCC_FUNC_##name)
>  
> +/* Hypercalls available only prior to pKVM finalisation */
>  #define __KVM_HOST_SMCCC_FUNC___kvm_hyp_init			0
> -#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			1
> -#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		2
> -#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		3
> -#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		4
> -#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		5
> -#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		6
> -#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			7
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		8
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		9
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		10
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		11
> -#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		12
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		13
> -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		14
> -#define __KVM_HOST_SMCCC_FUNC___pkvm_init			15
> -#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		16
> -#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	17
> -#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
> -#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
> -#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			20
> +#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		1
> +#define __KVM_HOST_SMCCC_FUNC___pkvm_init			2
> +#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	3
> +#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		4
> +#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			5
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		6
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		7
> +#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		8
> +
> +/* Hypercalls available after pKVM finalisation */
> +#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		9
> +#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			10
> +#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			11
> +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		12
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		13
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		14
> +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		15
> +#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		16
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		17
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		18
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		19
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		20
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2da6aa8da868..4120e34288e1 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -165,36 +165,42 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
>  #define HANDLE_FUNC(x)	[__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
>  
>  static const hcall_t host_hcall[] = {
> -	HANDLE_FUNC(__kvm_vcpu_run),
> +	/* ___kvm_hyp_init */
> +	HANDLE_FUNC(__kvm_get_mdcr_el2),
> +	HANDLE_FUNC(__pkvm_init),
> +	HANDLE_FUNC(__pkvm_create_private_mapping),
> +	HANDLE_FUNC(__pkvm_cpu_set_vector),
> +	HANDLE_FUNC(__kvm_enable_ssbs),
> +	HANDLE_FUNC(__vgic_v3_init_lrs),
> +	HANDLE_FUNC(__pkvm_prot_finalize),
> +
> +	HANDLE_FUNC(__pkvm_host_share_hyp),
>  	HANDLE_FUNC(__kvm_adjust_pc),
> +	HANDLE_FUNC(__kvm_vcpu_run),
>  	HANDLE_FUNC(__kvm_flush_vm_context),
>  	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
>  	HANDLE_FUNC(__kvm_tlb_flush_vmid),
>  	HANDLE_FUNC(__kvm_flush_cpu_context),
>  	HANDLE_FUNC(__kvm_timer_set_cntvoff),
> -	HANDLE_FUNC(__kvm_enable_ssbs),
>  	HANDLE_FUNC(__vgic_v3_get_gic_config),
>  	HANDLE_FUNC(__vgic_v3_read_vmcr),
>  	HANDLE_FUNC(__vgic_v3_write_vmcr),
> -	HANDLE_FUNC(__vgic_v3_init_lrs),
> -	HANDLE_FUNC(__kvm_get_mdcr_el2),
>  	HANDLE_FUNC(__vgic_v3_save_aprs),
>  	HANDLE_FUNC(__vgic_v3_restore_aprs),
> -	HANDLE_FUNC(__pkvm_init),
> -	HANDLE_FUNC(__pkvm_cpu_set_vector),
> -	HANDLE_FUNC(__pkvm_host_share_hyp),
> -	HANDLE_FUNC(__pkvm_create_private_mapping),
> -	HANDLE_FUNC(__pkvm_prot_finalize),
>  };
>  
>  static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
>  {
>  	DECLARE_REG(unsigned long, id, host_ctxt, 0);
> +	unsigned long hcall_min = 0;
>  	hcall_t hfn;
>  
> +	if (static_branch_unlikely(&kvm_protected_mode_initialized))
> +		hcall_min = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize;
> +
>  	id -= KVM_HOST_SMCCC_ID(0);
>  
> -	if (unlikely(id >= ARRAY_SIZE(host_hcall)))
> +	if (unlikely(id < hcall_min || id >= ARRAY_SIZE(host_hcall)))

So I can still issue a pkvm_prot_finalize after finalisation? Seems
odd. As hcall_min has to be inclusive, you probably want it to be set
to __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp once protected.

Thanks,

	M.
Will Deacon Sept. 23, 2021, 12:58 p.m. UTC | #2
On Thu, Sep 23, 2021 at 12:22:56PM +0100, Will Deacon wrote:
> After pKVM has been 'finalised' using the __pkvm_prot_finalize hypercall,
> the calling CPU will have a Stage-2 translation enabled to prevent access
> to memory pages owned by EL2.
> 
> Although this forms a significant part of the process to deprivilege the
> host kernel, we also need to ensure that the hypercall interface is
> reduced so that the EL2 code cannot, for example, be re-initialised using
> a new set of vectors.
> 
> Re-order the hypercalls so that only a suffix remains available after
> finalisation of pKVM.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_asm.h   | 43 ++++++++++++++++--------------
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 26 +++++++++++-------
>  2 files changed, 39 insertions(+), 30 deletions(-)

[...]

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2da6aa8da868..4120e34288e1 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -165,36 +165,42 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
>  #define HANDLE_FUNC(x)	[__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
>  
>  static const hcall_t host_hcall[] = {
> -	HANDLE_FUNC(__kvm_vcpu_run),
> +	/* ___kvm_hyp_init */
> +	HANDLE_FUNC(__kvm_get_mdcr_el2),
> +	HANDLE_FUNC(__pkvm_init),
> +	HANDLE_FUNC(__pkvm_create_private_mapping),
> +	HANDLE_FUNC(__pkvm_cpu_set_vector),
> +	HANDLE_FUNC(__kvm_enable_ssbs),
> +	HANDLE_FUNC(__vgic_v3_init_lrs),
> +	HANDLE_FUNC(__pkvm_prot_finalize),
> +
> +	HANDLE_FUNC(__pkvm_host_share_hyp),
>  	HANDLE_FUNC(__kvm_adjust_pc),
> +	HANDLE_FUNC(__kvm_vcpu_run),
>  	HANDLE_FUNC(__kvm_flush_vm_context),
>  	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
>  	HANDLE_FUNC(__kvm_tlb_flush_vmid),
>  	HANDLE_FUNC(__kvm_flush_cpu_context),
>  	HANDLE_FUNC(__kvm_timer_set_cntvoff),
> -	HANDLE_FUNC(__kvm_enable_ssbs),
>  	HANDLE_FUNC(__vgic_v3_get_gic_config),
>  	HANDLE_FUNC(__vgic_v3_read_vmcr),
>  	HANDLE_FUNC(__vgic_v3_write_vmcr),
> -	HANDLE_FUNC(__vgic_v3_init_lrs),
> -	HANDLE_FUNC(__kvm_get_mdcr_el2),
>  	HANDLE_FUNC(__vgic_v3_save_aprs),
>  	HANDLE_FUNC(__vgic_v3_restore_aprs),
> -	HANDLE_FUNC(__pkvm_init),
> -	HANDLE_FUNC(__pkvm_cpu_set_vector),
> -	HANDLE_FUNC(__pkvm_host_share_hyp),
> -	HANDLE_FUNC(__pkvm_create_private_mapping),
> -	HANDLE_FUNC(__pkvm_prot_finalize),

Not that it makes any functional difference, but I was trying to keep this
in numerical order and evidently didn't manage it after renumbering
__vgic_v3_get_gic_config. Will fix for v2.

Will
Will Deacon Sept. 23, 2021, 1:02 p.m. UTC | #3
On Thu, Sep 23, 2021 at 01:56:21PM +0100, Marc Zyngier wrote:
> On Thu, 23 Sep 2021 12:22:56 +0100,
> Will Deacon <will@kernel.org> wrote:
> > 
> > After pKVM has been 'finalised' using the __pkvm_prot_finalize hypercall,
> > the calling CPU will have a Stage-2 translation enabled to prevent access
> > to memory pages owned by EL2.
> > 
> > Although this forms a significant part of the process to deprivilege the
> > host kernel, we also need to ensure that the hypercall interface is
> > reduced so that the EL2 code cannot, for example, be re-initialised using
> > a new set of vectors.
> > 
> > Re-order the hypercalls so that only a suffix remains available after
> > finalisation of pKVM.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h   | 43 ++++++++++++++++--------------
> >  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 26 +++++++++++-------
> >  2 files changed, 39 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > index e86045ac43ba..68630fd382c5 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -43,27 +43,30 @@
> >  
> >  #define KVM_HOST_SMCCC_FUNC(name) KVM_HOST_SMCCC_ID(__KVM_HOST_SMCCC_FUNC_##name)
> >  
> > +/* Hypercalls available only prior to pKVM finalisation */
> >  #define __KVM_HOST_SMCCC_FUNC___kvm_hyp_init			0
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			1
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		2
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		3
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		4
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		5
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		6
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			7
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		8
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		9
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		10
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		11
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		12
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		13
> > -#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		14
> > -#define __KVM_HOST_SMCCC_FUNC___pkvm_init			15
> > -#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		16
> > -#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	17
> > -#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
> > -#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
> > -#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			20
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		1
> > +#define __KVM_HOST_SMCCC_FUNC___pkvm_init			2
> > +#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	3
> > +#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		4
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			5
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		6
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		7
> > +#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		8
> > +
> > +/* Hypercalls available after pKVM finalisation */
> > +#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		9
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			10
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			11
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		12
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		13
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		14
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		15
> > +#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		16
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		17
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		18
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		19
> > +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		20
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index 2da6aa8da868..4120e34288e1 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -165,36 +165,42 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
> >  #define HANDLE_FUNC(x)	[__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
> >  
> >  static const hcall_t host_hcall[] = {
> > -	HANDLE_FUNC(__kvm_vcpu_run),
> > +	/* ___kvm_hyp_init */
> > +	HANDLE_FUNC(__kvm_get_mdcr_el2),
> > +	HANDLE_FUNC(__pkvm_init),
> > +	HANDLE_FUNC(__pkvm_create_private_mapping),
> > +	HANDLE_FUNC(__pkvm_cpu_set_vector),
> > +	HANDLE_FUNC(__kvm_enable_ssbs),
> > +	HANDLE_FUNC(__vgic_v3_init_lrs),
> > +	HANDLE_FUNC(__pkvm_prot_finalize),
> > +
> > +	HANDLE_FUNC(__pkvm_host_share_hyp),
> >  	HANDLE_FUNC(__kvm_adjust_pc),
> > +	HANDLE_FUNC(__kvm_vcpu_run),
> >  	HANDLE_FUNC(__kvm_flush_vm_context),
> >  	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
> >  	HANDLE_FUNC(__kvm_tlb_flush_vmid),
> >  	HANDLE_FUNC(__kvm_flush_cpu_context),
> >  	HANDLE_FUNC(__kvm_timer_set_cntvoff),
> > -	HANDLE_FUNC(__kvm_enable_ssbs),
> >  	HANDLE_FUNC(__vgic_v3_get_gic_config),
> >  	HANDLE_FUNC(__vgic_v3_read_vmcr),
> >  	HANDLE_FUNC(__vgic_v3_write_vmcr),
> > -	HANDLE_FUNC(__vgic_v3_init_lrs),
> > -	HANDLE_FUNC(__kvm_get_mdcr_el2),
> >  	HANDLE_FUNC(__vgic_v3_save_aprs),
> >  	HANDLE_FUNC(__vgic_v3_restore_aprs),
> > -	HANDLE_FUNC(__pkvm_init),
> > -	HANDLE_FUNC(__pkvm_cpu_set_vector),
> > -	HANDLE_FUNC(__pkvm_host_share_hyp),
> > -	HANDLE_FUNC(__pkvm_create_private_mapping),
> > -	HANDLE_FUNC(__pkvm_prot_finalize),
> >  };
> >  
> >  static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
> >  {
> >  	DECLARE_REG(unsigned long, id, host_ctxt, 0);
> > +	unsigned long hcall_min = 0;
> >  	hcall_t hfn;
> >  
> > +	if (static_branch_unlikely(&kvm_protected_mode_initialized))
> > +		hcall_min = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize;
> > +
> >  	id -= KVM_HOST_SMCCC_ID(0);
> >  
> > -	if (unlikely(id >= ARRAY_SIZE(host_hcall)))
> > +	if (unlikely(id < hcall_min || id >= ARRAY_SIZE(host_hcall)))
> 
> So I can still issue a pkvm_prot_finalize after finalisation? Seems
> odd. As hcall_min has to be inclusive, you probably want it to be set
> to __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp once protected.

Yeah, I ended up addresing that one in the previous patch. The problem is
that we need to allow pkvm_prot_finalize to be called on each CPU, so I
think we'd end up having an extra "really finalize damnit!" call to be
issued _once_ after each CPU is done with the finalisation if we want
to lock it down.

The approach I took instead is to make pkvm_prot_finalize return -EBUSY
if it's called on a CPU where it's already been called.

Will
Marc Zyngier Sept. 23, 2021, 1:11 p.m. UTC | #4
On Thu, 23 Sep 2021 14:02:11 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Thu, Sep 23, 2021 at 01:56:21PM +0100, Marc Zyngier wrote:
> > On Thu, 23 Sep 2021 12:22:56 +0100,
> > Will Deacon <will@kernel.org> wrote:

[...]

> > >  static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
> > >  {
> > >  	DECLARE_REG(unsigned long, id, host_ctxt, 0);
> > > +	unsigned long hcall_min = 0;
> > >  	hcall_t hfn;
> > >  
> > > +	if (static_branch_unlikely(&kvm_protected_mode_initialized))
> > > +		hcall_min = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize;
> > > +
> > >  	id -= KVM_HOST_SMCCC_ID(0);
> > >  
> > > -	if (unlikely(id >= ARRAY_SIZE(host_hcall)))
> > > +	if (unlikely(id < hcall_min || id >= ARRAY_SIZE(host_hcall)))
> > 
> > So I can still issue a pkvm_prot_finalize after finalisation? Seems
> > odd. As hcall_min has to be inclusive, you probably want it to be set
> > to __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp once protected.
> 
> Yeah, I ended up addresing that one in the previous patch. The problem is
> that we need to allow pkvm_prot_finalize to be called on each CPU, so I
> think we'd end up having an extra "really finalize damnit!" call to be
> issued _once_ after each CPU is done with the finalisation if we want
> to lock it down.
> 
> The approach I took instead is to make pkvm_prot_finalize return -EBUSY
> if it's called on a CPU where it's already been called.

Ah, I see. Serves me right for reading patches out of order. Finalise
is of course per-CPU, and the static key global. Epic fail.

Probably deserves a comment, because I'm surely going to jump at that
again in three months.

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index e86045ac43ba..68630fd382c5 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -43,27 +43,30 @@ 
 
 #define KVM_HOST_SMCCC_FUNC(name) KVM_HOST_SMCCC_ID(__KVM_HOST_SMCCC_FUNC_##name)
 
+/* Hypercalls available only prior to pKVM finalisation */
 #define __KVM_HOST_SMCCC_FUNC___kvm_hyp_init			0
-#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			1
-#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		2
-#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		3
-#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		4
-#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		5
-#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		6
-#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			7
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		8
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		9
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		10
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		11
-#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		12
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		13
-#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		14
-#define __KVM_HOST_SMCCC_FUNC___pkvm_init			15
-#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		16
-#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	17
-#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
-#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
-#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			20
+#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2		1
+#define __KVM_HOST_SMCCC_FUNC___pkvm_init			2
+#define __KVM_HOST_SMCCC_FUNC___pkvm_create_private_mapping	3
+#define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		4
+#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs			5
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs		6
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_gic_config		7
+#define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		8
+
+/* Hypercalls available after pKVM finalisation */
+#define __KVM_HOST_SMCCC_FUNC___pkvm_host_share_hyp		9
+#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			10
+#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run			11
+#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context		12
+#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa		13
+#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid		14
+#define __KVM_HOST_SMCCC_FUNC___kvm_flush_cpu_context		15
+#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff		16
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr		17
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr		18
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs		19
+#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs		20
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 2da6aa8da868..4120e34288e1 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -165,36 +165,42 @@  typedef void (*hcall_t)(struct kvm_cpu_context *);
 #define HANDLE_FUNC(x)	[__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
 
 static const hcall_t host_hcall[] = {
-	HANDLE_FUNC(__kvm_vcpu_run),
+	/* ___kvm_hyp_init */
+	HANDLE_FUNC(__kvm_get_mdcr_el2),
+	HANDLE_FUNC(__pkvm_init),
+	HANDLE_FUNC(__pkvm_create_private_mapping),
+	HANDLE_FUNC(__pkvm_cpu_set_vector),
+	HANDLE_FUNC(__kvm_enable_ssbs),
+	HANDLE_FUNC(__vgic_v3_init_lrs),
+	HANDLE_FUNC(__pkvm_prot_finalize),
+
+	HANDLE_FUNC(__pkvm_host_share_hyp),
 	HANDLE_FUNC(__kvm_adjust_pc),
+	HANDLE_FUNC(__kvm_vcpu_run),
 	HANDLE_FUNC(__kvm_flush_vm_context),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid),
 	HANDLE_FUNC(__kvm_flush_cpu_context),
 	HANDLE_FUNC(__kvm_timer_set_cntvoff),
-	HANDLE_FUNC(__kvm_enable_ssbs),
 	HANDLE_FUNC(__vgic_v3_get_gic_config),
 	HANDLE_FUNC(__vgic_v3_read_vmcr),
 	HANDLE_FUNC(__vgic_v3_write_vmcr),
-	HANDLE_FUNC(__vgic_v3_init_lrs),
-	HANDLE_FUNC(__kvm_get_mdcr_el2),
 	HANDLE_FUNC(__vgic_v3_save_aprs),
 	HANDLE_FUNC(__vgic_v3_restore_aprs),
-	HANDLE_FUNC(__pkvm_init),
-	HANDLE_FUNC(__pkvm_cpu_set_vector),
-	HANDLE_FUNC(__pkvm_host_share_hyp),
-	HANDLE_FUNC(__pkvm_create_private_mapping),
-	HANDLE_FUNC(__pkvm_prot_finalize),
 };
 
 static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
 {
 	DECLARE_REG(unsigned long, id, host_ctxt, 0);
+	unsigned long hcall_min = 0;
 	hcall_t hfn;
 
+	if (static_branch_unlikely(&kvm_protected_mode_initialized))
+		hcall_min = __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize;
+
 	id -= KVM_HOST_SMCCC_ID(0);
 
-	if (unlikely(id >= ARRAY_SIZE(host_hcall)))
+	if (unlikely(id < hcall_min || id >= ARRAY_SIZE(host_hcall)))
 		goto inval;
 
 	hfn = host_hcall[id];