Re: [PATCH v3 04/18] KVM: arm64: Restrict symbol aliasing to outside nVHE

From: Andrew Scull <ascull@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kernel-team@android.com, catalin.marinas@arm.com,
	linux-arm-kernel@lists.infradead.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	will@kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v3 04/18] KVM: arm64: Restrict symbol aliasing to outside nVHE
Date: Tue, 8 Sep 2020 11:13:00 +0100	[thread overview]
Message-ID: <20200908101300.GA3268721@google.com> (raw)
In-Reply-To: <87zh61lvm9.wl-maz@kernel.org>

On Mon, Sep 07, 2020 at 11:38:38AM +0100, Marc Zyngier wrote:
> Hi Andrew,
> 
> On Thu, 03 Sep 2020 14:52:53 +0100,
> Andrew Scull <ascull@google.com> wrote:
> > 
> > nVHE symbols are prefixed but this is sometimes hidden from the host by
> > aliasing the non-prefixed symbol to the prefixed version with a macro.
> > This runs into problems if nVHE tries to use the symbol as it becomes
> > doubly prefixed. Avoid this by omitting the aliasing macro for nVHE.
> > 
> > Cc: David Brazdil <dbrazdil@google.com>
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > index 6f98fbd0ac81..6f9c4162a764 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -99,8 +99,11 @@ struct kvm_s2_mmu;
> >  
> >  DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
> >  DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
> > +
> > +#ifndef __KVM_NVHE_HYPERVISOR__
> >  #define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
> >  #define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
> > +#endif
> 
> Hmmm. Why do we limit this to these two symbols instead of making it a
> property of the "CHOOSE_*" implementation?
> 
> The use of CHOOSE_HYP_SYM is already forbidden in the EL2 code (see
> how any symbol results in __nvhe_undefined_symbol being emitted). Does
> anything break if we have:
> 
> #define CHOOSE_NVHE_SYM(x)	x
> 
> when __KVM_NVHE_HYPERVISOR__ is defined?

I've specialized the CHOOSE_* macros along the lines you suggested for
each of the 3 relevant contexts: host, VHE and nVHE. If you think that's
overkill, the host and VHE cases can be merged.

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 6f98fbd0ac81..a952859117b2 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -60,10 +60,24 @@
 	DECLARE_KVM_VHE_SYM(sym);		\
 	DECLARE_KVM_NVHE_SYM(sym)
 
+#if defined(__KVM_NVHE_HYPERVISOR__)
+
+#define CHOOSE_HYP_SYM(sym)	CHOOSE_NVHE_SYM(sym)
+#define CHOOSE_NVHE_SYM(sym)	sym
+/* The nVHE hypervisor shouldn't even try to access VHE symbols */
+extern void *__nvhe_undefined_symbol;
+#define CHOOSE_VHE_SYM(sym)	__nvhe_undefined_symbol
+
+#elif defined(__KVM_VHE_HYPERVISOR)
+
+#define CHOOSE_HYP_SYM(sym)	CHOOSE_VHE_SYM(sym)
 #define CHOOSE_VHE_SYM(sym)	sym
-#define CHOOSE_NVHE_SYM(sym)	kvm_nvhe_sym(sym)
+/* The VHE hypervisor shouldn't even try to access nVHE symbols */
+extern void *__vhe_undefined_symbol;
+#define CHOOSE_NVHE_SYM(sym)	__vhe_undefined_symbol
+
+#else
 
-#ifndef __KVM_NVHE_HYPERVISOR__
 /*
  * BIG FAT WARNINGS:
  *
@@ -77,10 +91,9 @@
  */
 #define CHOOSE_HYP_SYM(sym)	(is_kernel_in_hyp_mode() ? CHOOSE_VHE_SYM(sym) \
 					   : CHOOSE_NVHE_SYM(sym))
-#else
-/* The nVHE hypervisor shouldn't even try to access anything */
-extern void *__nvhe_undefined_symbol;
-#define CHOOSE_HYP_SYM(sym)	__nvhe_undefined_symbol
+#define CHOOSE_VHE_SYM(sym)	sym
+#define CHOOSE_NVHE_SYM(sym)	kvm_nvhe_sym(sym)
+
 #endif
 
 /* Translate a kernel address @ptr into its equivalent linear mapping */
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
