* [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest
@ 2019-01-28 6:58 Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys Amit Daniel Kachhap
` (5 more replies)
0 siblings, 6 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
Hi,
This patch series adds pointer authentication support for KVM guest and
is based on top of Linux 5.0-rc3. The basic patches in this series was
originally posted by Mark Rutland earlier[1,2] and contains some history
of this work.
Extension Overview:
=============================================
The ARMv8.3 pointer authentication extension adds functionality to detect
modification of pointer values, mitigating certain classes of attack such as
stack smashing, and making return oriented programming attacks harder.
The extension introduces the concept of a pointer authentication code (PAC),
which is stored in some upper bits of pointers. Each PAC is derived from the
original pointer, another 64-bit value (e.g. the stack pointer), and a secret
128-bit key.
New instructions are added which can be used to:
* Insert a PAC into a pointer
* Strip a PAC from a pointer
* Authenticate and strip a PAC from a pointer
The detailed description of ARMv8.3 pointer authentication support in
userspace/kernel and can be found in Kristina's generic pointer authentication
patch series[3].
KVM guest work:
==============================================
If pointer authentication is enabled for KVM guests then the new PAC instructions
will not trap to EL2. If not then they may be ignored if in HINT region or trapped
in EL2 as illegal instruction. Since KVM guest vcpu runs as a thread so they have
a key initialized which will be used by PAC. When world switch happens between
host and guest then this key is exchanged.
There were some review comments by Christoffer Dall in the original series [1,2,3]
and this patch series tries to implement them. The original series enabled pointer
authentication for both userspace and kvm userspace. However it is now
bifurcated and this series contains only KVM guest support.
The current v5 patch series contains most of the suggestion by James Morse.
One of the suggestions was to do save/restore keys in asm during switch. However
this series re-uses the arm64 core functions __ptrauth_key_install but called from
C function using __no_ptrauth attribute.
Changes since v4 [6]: Several suggestions from James Morse
* Move host registers to be saved/restored inside struct kvm_cpu_context.
* Similar to hcr_el2, save/restore mdcr_el2 register also.
* Added save routines for ptrauth keys in generic arm core and
use them during KVM context switch.
* Defined a GCC attribute __no_ptrauth which discards generating
ptrauth instructions in a function. This is taken from Kristina's
earlier kernel pointer authentication support patches [4].
* Dropped a patch to mask cpufeature when not enabled from userspace and
now only key registers are masked from register list.
Changes since v3 [5]:
* Use pointer authentication only when VHE is present as ARM8.3 implies ARM8.1
features to be present.
* Added lazy context handling of ptrauth instructions from V2 version again.
* Added more details in Documentation.
Changes since v2 [1,2]:
* Allow host and guest to have different HCR_EL2 settings and not just constant
value HCR_HOST_VHE_FLAGS or HCR_HOST_NVHE_FLAGS.
* Optimise the reading of HCR_EL2 in host/guest switch by fetching it once
during KVM initialisation state and using it later.
* Context switch pointer authentication keys when switching between guest
and host. Pointer authentication was enabled in a lazy context earlier[2] and
is removed now to make it simple. However it can be revisited later if there
is significant performance issue.
* Added a userspace option to choose pointer authentication.
* Based on the userspace option, ptrauth cpufeature will be visible.
* Based on the userspace option, ptrauth key registers will be accessible.
* A small document is added on how to enable pointer authentication from
userspace KVM API.
Looking for feedback and comments.
Thanks,
Amit
[1]: https://lore.kernel.org/lkml/20171127163806.31435-11-mark.rutland@arm.com/
[2]: https://lore.kernel.org/lkml/20171127163806.31435-10-mark.rutland@arm.com/
[3]: https://lkml.org/lkml/2018/12/7/666
[4]: https://lore.kernel.org/lkml/20181005084754.20950-1-kristina.martsenko@arm.com/
[5]: https://lkml.org/lkml/2018/10/17/594
[6]: https://lkml.org/lkml/2018/12/18/80
Linux (5.0-rc3 based):
Amit Daniel Kachhap (5):
arm64: Add utilities to save restore pointer authentication keys
arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
arm64/kvm: context-switch ptrauth registers
arm64/kvm: add a userspace option to enable pointer authentication
arm64/kvm: control accessibility of ptrauth key registers
Documentation/arm64/pointer-authentication.txt | 12 +++--
Documentation/virtual/kvm/api.txt | 4 ++
arch/arm/include/asm/kvm_host.h | 8 ++-
arch/arm64/include/asm/cpufeature.h | 5 ++
arch/arm64/include/asm/kvm_asm.h | 2 +
arch/arm64/include/asm/kvm_emulate.h | 22 ++++----
arch/arm64/include/asm/kvm_host.h | 55 +++++++++++++++++---
arch/arm64/include/asm/kvm_hyp.h | 9 +++-
arch/arm64/include/asm/pointer_auth.h | 28 +++++++++++
arch/arm64/include/uapi/asm/kvm.h | 1 +
arch/arm64/kernel/traps.c | 1 +
arch/arm64/kvm/debug.c | 28 +++--------
arch/arm64/kvm/guest.c | 2 +-
arch/arm64/kvm/handle_exit.c | 24 +++++----
arch/arm64/kvm/hyp/Makefile | 1 +
arch/arm64/kvm/hyp/ptrauth-sr.c | 57 +++++++++++++++++++++
arch/arm64/kvm/hyp/switch.c | 44 ++++++++--------
arch/arm64/kvm/hyp/sysreg-sr.c | 13 ++++-
arch/arm64/kvm/hyp/tlb.c | 6 ++-
arch/arm64/kvm/reset.c | 3 ++
arch/arm64/kvm/sys_regs.c | 70 +++++++++++++++++++++-----
include/uapi/linux/kvm.h | 1 +
virt/kvm/arm/arm.c | 6 ++-
23 files changed, 307 insertions(+), 95 deletions(-)
create mode 100644 arch/arm64/kvm/hyp/ptrauth-sr.c
kvmtool:
Repo: git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
Amit Daniel Kachhap (1):
arm/kvm: arm64: Add a vcpu feature for pointer authentication
arm/aarch32/include/kvm/kvm-cpu-arch.h | 2 ++
arm/aarch64/include/asm/kvm.h | 3 +++
arm/aarch64/include/kvm/kvm-arch.h | 1 +
arm/aarch64/include/kvm/kvm-config-arch.h | 4 +++-
arm/aarch64/include/kvm/kvm-cpu-arch.h | 2 ++
arm/aarch64/kvm-cpu.c | 5 +++++
arm/include/arm-common/kvm-config-arch.h | 1 +
arm/kvm-cpu.c | 7 +++++++
include/linux/kvm.h | 1 +
9 files changed, 25 insertions(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-01-31 16:20 ` James Morse
2019-01-28 6:58 ` [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value Amit Daniel Kachhap
` (4 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
The keys can be switched either inside an assembly or such
functions which do not have pointer authentication checks, so a GCC
attribute is added to enable it.
A function ptrauth_keys_store is added which is similar to existing
function ptrauth_keys_switch but saves the key values in memory.
This may be useful for save/restore scenarios when CPU changes
privilege levels, suspend/resume etc.
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm64/include/asm/pointer_auth.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
index 15d4951..98441ce 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -11,6 +11,13 @@
#ifdef CONFIG_ARM64_PTR_AUTH
/*
+ * Compile the function without pointer authentication instructions. This
+ * allows pointer authentication to be enabled/disabled within the function
+ * (but leaves the function unprotected by pointer authentication).
+ */
+#define __no_ptrauth __attribute__((target("sign-return-address=none")))
+
+/*
* Each key is a 128-bit quantity which is split across a pair of 64-bit
* registers (Lo and Hi).
*/
@@ -50,6 +57,13 @@ do { \
write_sysreg_s(__pki_v.hi, SYS_ ## k ## KEYHI_EL1); \
} while (0)
+#define __ptrauth_key_save(k, v) \
+do { \
+ struct ptrauth_key __pki_v = (v); \
+ __pki_v.lo = read_sysreg_s(SYS_ ## k ## KEYLO_EL1); \
+ __pki_v.hi = read_sysreg_s(SYS_ ## k ## KEYHI_EL1); \
+} while (0)
+
static inline void ptrauth_keys_switch(struct ptrauth_keys *keys)
{
if (system_supports_address_auth()) {
@@ -63,6 +77,19 @@ static inline void ptrauth_keys_switch(struct ptrauth_keys *keys)
__ptrauth_key_install(APGA, keys->apga);
}
+static inline void ptrauth_keys_store(struct ptrauth_keys *keys)
+{
+ if (system_supports_address_auth()) {
+ __ptrauth_key_save(APIA, keys->apia);
+ __ptrauth_key_save(APIB, keys->apib);
+ __ptrauth_key_save(APDA, keys->apda);
+ __ptrauth_key_save(APDB, keys->apdb);
+ }
+
+ if (system_supports_generic_auth())
+ __ptrauth_key_save(APGA, keys->apga);
+}
+
extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg);
/*
@@ -88,6 +115,7 @@ do { \
ptrauth_keys_switch(&(tsk)->thread.keys_user)
#else /* CONFIG_ARM64_PTR_AUTH */
+#define __no_ptrauth
#define ptrauth_prctl_reset_keys(tsk, arg) (-EINVAL)
#define ptrauth_strip_insn_pac(lr) (lr)
#define ptrauth_thread_init_user(tsk)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-01-31 16:22 ` James Morse
2019-02-13 17:34 ` Kristina Martsenko
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
` (3 subsequent siblings)
5 siblings, 2 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
is a constant value. This works today, as the host HCR_EL2 value is
always the same, but this will get in the way of supporting extensions
that require HCR_EL2 bits to be set conditionally for the host.
To allow such features to work without KVM having to explicitly handle
every possible host feature combination, this patch has KVM save/restore
the host HCR when switching to/from a guest HCR. The saving of the
register is done once during cpu hypervisor initialization state and is
just restored after switch from guest.
For fetching HCR_EL2 during kvm initialisation, a hyp call is made using
kvm_call_hyp and is helpful in NHVE case.
For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated
to toggle the TGE bit with a RMW sequence, as we already do in
__tlb_switch_to_guest_vhe().
While at it, host MDCR_EL2 value is fetched in a similar way and restored
after every switch from host to guest. There should not be any change in
functionality due to this.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm/include/asm/kvm_host.h | 3 ++-
arch/arm64/include/asm/kvm_asm.h | 2 ++
arch/arm64/include/asm/kvm_emulate.h | 22 ++++++++++----------
arch/arm64/include/asm/kvm_host.h | 28 ++++++++++++++++++++-----
arch/arm64/include/asm/kvm_hyp.h | 2 +-
arch/arm64/kvm/debug.c | 28 ++++++-------------------
arch/arm64/kvm/guest.c | 2 +-
arch/arm64/kvm/hyp/switch.c | 40 +++++++++++++++---------------------
arch/arm64/kvm/hyp/sysreg-sr.c | 13 +++++++++++-
arch/arm64/kvm/hyp/tlb.c | 6 +++++-
virt/kvm/arm/arm.c | 4 ++--
11 files changed, 82 insertions(+), 68 deletions(-)
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ca56537..704667e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void)
kvm_call_hyp(__init_stage2_translation);
}
+static inline void __cpu_copy_hyp_conf(void) {}
+
static inline int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
{
return 0;
@@ -292,7 +294,6 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
-static inline void kvm_arm_init_debug(void) {}
static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index f5b79e9..2da6e43 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void);
extern u32 __kvm_get_mdcr_el2(void);
+extern u64 __kvm_get_hcr_el2(void);
+
/* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */
#define __hyp_this_cpu_ptr(sym) \
({ \
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 506386a..0dbe795 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -50,25 +50,25 @@ void kvm_inject_pabt32(struct kvm_vcpu *vcpu, unsigned long addr);
static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
{
- return !(vcpu->arch.hcr_el2 & HCR_RW);
+ return !(vcpu->arch.ctxt.hcr_el2 & HCR_RW);
}
static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
{
- vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
+ vcpu->arch.ctxt.hcr_el2 = HCR_GUEST_FLAGS;
if (is_kernel_in_hyp_mode())
- vcpu->arch.hcr_el2 |= HCR_E2H;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_E2H;
if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN)) {
/* route synchronous external abort exceptions to EL2 */
- vcpu->arch.hcr_el2 |= HCR_TEA;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_TEA;
/* trap error record accesses */
- vcpu->arch.hcr_el2 |= HCR_TERR;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_TERR;
}
if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
- vcpu->arch.hcr_el2 |= HCR_FWB;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_FWB;
if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
- vcpu->arch.hcr_el2 &= ~HCR_RW;
+ vcpu->arch.ctxt.hcr_el2 &= ~HCR_RW;
/*
* TID3: trap feature register accesses that we virtualise.
@@ -76,22 +76,22 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
* are currently virtualised.
*/
if (!vcpu_el1_is_32bit(vcpu))
- vcpu->arch.hcr_el2 |= HCR_TID3;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_TID3;
}
static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
{
- return (unsigned long *)&vcpu->arch.hcr_el2;
+ return (unsigned long *)&vcpu->arch.ctxt.hcr_el2;
}
static inline void vcpu_clear_wfe_traps(struct kvm_vcpu *vcpu)
{
- vcpu->arch.hcr_el2 &= ~HCR_TWE;
+ vcpu->arch.ctxt.hcr_el2 &= ~HCR_TWE;
}
static inline void vcpu_set_wfe_traps(struct kvm_vcpu *vcpu)
{
- vcpu->arch.hcr_el2 |= HCR_TWE;
+ vcpu->arch.ctxt.hcr_el2 |= HCR_TWE;
}
static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7732d0b..1f2d237 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -203,6 +203,10 @@ struct kvm_cpu_context {
u32 copro[NR_COPRO_REGS];
};
+ /* HYP configuration */
+ u64 hcr_el2;
+ u32 mdcr_el2;
+
struct kvm_vcpu *__hyp_running_vcpu;
};
@@ -211,10 +215,6 @@ typedef struct kvm_cpu_context kvm_cpu_context_t;
struct kvm_vcpu_arch {
struct kvm_cpu_context ctxt;
- /* HYP configuration */
- u64 hcr_el2;
- u32 mdcr_el2;
-
/* Exception Information */
struct kvm_vcpu_fault_info fault;
@@ -445,7 +445,6 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {}
-void kvm_arm_init_debug(void);
void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
@@ -458,6 +457,25 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
static inline void __cpu_init_stage2(void) {}
+/**
+ * __cpu_copy_hyp_conf - copy the boot hyp configuration registers
+ *
+ * It is called once per-cpu during CPU hyp initialisation.
+ */
+static inline void __cpu_copy_hyp_conf(void)
+{
+ kvm_cpu_context_t *host_cxt = this_cpu_ptr(&kvm_host_cpu_state);
+
+ host_cxt->hcr_el2 = kvm_call_hyp(__kvm_get_hcr_el2);
+
+ /*
+ * Retrieve the initial value of mdcr_el2 so we can preserve
+ * MDCR_EL2.HPMN which has presumably been set-up by some
+ * knowledgeable bootcode.
+ */
+ host_cxt->mdcr_el2 = kvm_call_hyp(__kvm_get_mdcr_el2);
+}
+
/* Guest/host FPSIMD coordination helpers */
int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index a80a7ef..6e65cad 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -151,7 +151,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
bool __fpsimd_enabled(void);
void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
-void deactivate_traps_vhe_put(void);
+void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu);
u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
void __noreturn __hyp_do_panic(unsigned long, ...);
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index f39801e..99dc0a4 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -32,8 +32,6 @@
DBG_MDSCR_KDE | \
DBG_MDSCR_MDE)
-static DEFINE_PER_CPU(u32, mdcr_el2);
-
/**
* save/restore_guest_debug_regs
*
@@ -65,21 +63,6 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
}
/**
- * kvm_arm_init_debug - grab what we need for debug
- *
- * Currently the sole task of this function is to retrieve the initial
- * value of mdcr_el2 so we can preserve MDCR_EL2.HPMN which has
- * presumably been set-up by some knowledgeable bootcode.
- *
- * It is called once per-cpu during CPU hyp initialisation.
- */
-
-void kvm_arm_init_debug(void)
-{
- __this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
-}
-
-/**
* kvm_arm_reset_debug_ptr - reset the debug ptr to point to the vcpu state
*/
@@ -111,6 +94,7 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
{
+ kvm_cpu_context_t *host_cxt = this_cpu_ptr(&kvm_host_cpu_state);
bool trap_debug = !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY);
unsigned long mdscr;
@@ -120,8 +104,8 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
* This also clears MDCR_EL2_E2PB_MASK to disable guest access
* to the profiling buffer.
*/
- vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
- vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
+ vcpu->arch.ctxt.mdcr_el2 = host_cxt->mdcr_el2 & MDCR_EL2_HPMN_MASK;
+ vcpu->arch.ctxt.mdcr_el2 |= (MDCR_EL2_TPM |
MDCR_EL2_TPMS |
MDCR_EL2_TPMCR |
MDCR_EL2_TDRA |
@@ -130,7 +114,7 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
/* Is Guest debugging in effect? */
if (vcpu->guest_debug) {
/* Route all software debug exceptions to EL2 */
- vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
+ vcpu->arch.ctxt.mdcr_el2 |= MDCR_EL2_TDE;
/* Save guest debug state */
save_guest_debug_regs(vcpu);
@@ -202,13 +186,13 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
/* Trap debug register access */
if (trap_debug)
- vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
+ vcpu->arch.ctxt.mdcr_el2 |= MDCR_EL2_TDA;
/* If KDE or MDE are set, perform a full save/restore cycle. */
if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
vcpu->arch.flags |= KVM_ARM64_DEBUG_DIRTY;
- trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
+ trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.ctxt.mdcr_el2);
trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_read_sys_reg(vcpu, MDSCR_EL1));
}
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index dd436a5..e2f0268 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -345,7 +345,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
struct kvm_vcpu_events *events)
{
- events->exception.serror_pending = !!(vcpu->arch.hcr_el2 & HCR_VSE);
+ events->exception.serror_pending = !!(vcpu->arch.ctxt.hcr_el2 & HCR_VSE);
events->exception.serror_has_esr = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
if (events->exception.serror_pending && events->exception.serror_has_esr)
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index b0b1478..03b36f1 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -82,7 +82,7 @@ static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
*/
write_sysreg(0, pmselr_el0);
write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
- write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
+ write_sysreg(vcpu->arch.ctxt.mdcr_el2, mdcr_el2);
}
static void __hyp_text __deactivate_traps_common(void)
@@ -126,7 +126,7 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
{
- u64 hcr = vcpu->arch.hcr_el2;
+ u64 hcr = vcpu->arch.ctxt.hcr_el2;
write_sysreg(hcr, hcr_el2);
@@ -139,10 +139,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
__activate_traps_nvhe(vcpu);
}
-static void deactivate_traps_vhe(void)
+static void deactivate_traps_vhe(struct kvm_cpu_context *host_ctxt)
{
extern char vectors[]; /* kernel exception vectors */
- write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
+ write_sysreg(host_ctxt->hcr_el2, hcr_el2);
/*
* ARM erratum 1165522 requires the actual execution of the above
@@ -155,35 +155,33 @@ static void deactivate_traps_vhe(void)
write_sysreg(vectors, vbar_el1);
}
-static void __hyp_text __deactivate_traps_nvhe(void)
+static void __hyp_text __deactivate_traps_nvhe(struct kvm_cpu_context *host_ctxt)
{
- u64 mdcr_el2 = read_sysreg(mdcr_el2);
-
__deactivate_traps_common();
- mdcr_el2 &= MDCR_EL2_HPMN_MASK;
- mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
-
- write_sysreg(mdcr_el2, mdcr_el2);
- write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2);
+ write_sysreg(host_ctxt->mdcr_el2, mdcr_el2);
+ write_sysreg(host_ctxt->hcr_el2, hcr_el2);
write_sysreg(CPTR_EL2_DEFAULT, cptr_el2);
}
static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
{
+ struct kvm_cpu_context *host_ctxt;
+
+ host_ctxt = vcpu->arch.host_cpu_context;
/*
* If we pended a virtual abort, preserve it until it gets
* cleared. See D1.14.3 (Virtual Interrupts) for details, but
* the crucial bit is "On taking a vSError interrupt,
* HCR_EL2.VSE is cleared to 0."
*/
- if (vcpu->arch.hcr_el2 & HCR_VSE)
- vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
+ if (vcpu->arch.ctxt.hcr_el2 & HCR_VSE)
+ vcpu->arch.ctxt.hcr_el2 = read_sysreg(hcr_el2);
if (has_vhe())
- deactivate_traps_vhe();
+ deactivate_traps_vhe(host_ctxt);
else
- __deactivate_traps_nvhe();
+ __deactivate_traps_nvhe(host_ctxt);
}
void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
@@ -191,15 +189,11 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
__activate_traps_common(vcpu);
}
-void deactivate_traps_vhe_put(void)
+void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu)
{
- u64 mdcr_el2 = read_sysreg(mdcr_el2);
-
- mdcr_el2 &= MDCR_EL2_HPMN_MASK |
- MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
- MDCR_EL2_TPMS;
+ struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
- write_sysreg(mdcr_el2, mdcr_el2);
+ write_sysreg(host_ctxt->mdcr_el2, mdcr_el2);
__deactivate_traps_common();
}
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 68d6f7c..22c854a 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -294,7 +294,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
if (!has_vhe())
return;
- deactivate_traps_vhe_put();
+ deactivate_traps_vhe_put(vcpu);
__sysreg_save_el1_state(guest_ctxt);
__sysreg_save_user_state(guest_ctxt);
@@ -316,3 +316,14 @@ void __hyp_text __kvm_enable_ssbs(void)
"msr sctlr_el2, %0"
: "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS));
}
+
+/**
+ * __read_hyp_hcr_el2 - Returns hcr_el2 register value
+ *
+ * This function acts as a function handler parameter for kvm_call_hyp and
+ * may be called from EL1 exception level to fetch the register value.
+ */
+u64 __hyp_text __kvm_get_hcr_el2(void)
+{
+ return read_sysreg(hcr_el2);
+}
diff --git a/arch/arm64/kvm/hyp/tlb.c b/arch/arm64/kvm/hyp/tlb.c
index 76c3086..c5e7144 100644
--- a/arch/arm64/kvm/hyp/tlb.c
+++ b/arch/arm64/kvm/hyp/tlb.c
@@ -86,12 +86,16 @@ static hyp_alternate_select(__tlb_switch_to_guest,
static void __hyp_text __tlb_switch_to_host_vhe(struct kvm *kvm,
struct tlb_inv_context *cxt)
{
+ u64 val;
+
/*
* We're done with the TLB operation, let's restore the host's
* view of HCR_EL2.
*/
write_sysreg(0, vttbr_el2);
- write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
+ val = read_sysreg(hcr_el2);
+ val |= HCR_TGE;
+ write_sysreg(val, hcr_el2);
isb();
if (cpus_have_const_cap(ARM64_WORKAROUND_1165522)) {
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9e350fd3..2d65ada 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1327,10 +1327,10 @@ static void cpu_hyp_reinit(void)
else
cpu_init_hyp_mode(NULL);
- kvm_arm_init_debug();
-
if (vgic_present)
kvm_vgic_init_cpu_hardware();
+
+ __cpu_copy_hyp_conf();
}
static void _kvm_arch_hardware_enable(void *discard)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-01-28 14:25 ` Julien Thierry
` (2 more replies)
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
` (2 subsequent siblings)
5 siblings, 3 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
When pointer authentication is supported, a guest may wish to use it.
This patch adds the necessary KVM infrastructure for this to work, with
a semi-lazy context switch of the pointer auth state.
Pointer authentication feature is only enabled when VHE is built
into the kernel and present into CPU implementation so only VHE code
paths are modified.
When we schedule a vcpu, we disable guest usage of pointer
authentication instructions and accesses to the keys. While these are
disabled, we avoid context-switching the keys. When we trap the guest
trying to use pointer authentication functionality, we change to eagerly
context-switching the keys, and enable the feature. The next time the
vcpu is scheduled out/in, we start again.
Pointer authentication consists of address authentication and generic
authentication, and CPUs in a system might have varied support for
either. Where support for either feature is not uniform, it is hidden
from guests via ID register emulation, as a result of the cpufeature
framework in the host.
Unfortunately, address authentication and generic authentication cannot
be trapped separately, as the architecture provides a single EL2 trap
covering both. If we wish to expose one without the other, we cannot
prevent a (badly-written) guest from intermittently using a feature
which is not uniformly supported (when scheduled on a physical CPU which
supports the relevant feature). When the guest is scheduled on a
physical CPU lacking the feature, these attempts will result in an UNDEF
being taken by the guest.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm/include/asm/kvm_host.h | 1 +
arch/arm64/include/asm/cpufeature.h | 5 +++++
arch/arm64/include/asm/kvm_host.h | 24 ++++++++++++++++++++
arch/arm64/include/asm/kvm_hyp.h | 7 ++++++
arch/arm64/kernel/traps.c | 1 +
arch/arm64/kvm/handle_exit.c | 23 +++++++++++--------
arch/arm64/kvm/hyp/Makefile | 1 +
arch/arm64/kvm/hyp/ptrauth-sr.c | 44 +++++++++++++++++++++++++++++++++++++
arch/arm64/kvm/hyp/switch.c | 4 ++++
arch/arm64/kvm/sys_regs.c | 40 ++++++++++++++++++++++++++-------
virt/kvm/arm/arm.c | 2 ++
11 files changed, 135 insertions(+), 17 deletions(-)
create mode 100644 arch/arm64/kvm/hyp/ptrauth-sr.c
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 704667e..b200c14 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -345,6 +345,7 @@ static inline int kvm_arm_have_ssbd(void)
static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
+static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
#define __KVM_HAVE_ARCH_VM_ALLOC
struct kvm *kvm_arch_alloc_vm(void);
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index dfcfba7..e1bf2a5 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
}
+static inline bool kvm_supports_ptrauth(void)
+{
+ return system_supports_address_auth() && system_supports_generic_auth();
+}
+
#define ARM64_SSBD_UNKNOWN -1
#define ARM64_SSBD_FORCE_DISABLE 0
#define ARM64_SSBD_KERNEL 1
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1f2d237..c798d0c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -146,6 +146,18 @@ enum vcpu_sysreg {
PMSWINC_EL0, /* Software Increment Register */
PMUSERENR_EL0, /* User Enable Register */
+ /* Pointer Authentication Registers */
+ APIAKEYLO_EL1,
+ APIAKEYHI_EL1,
+ APIBKEYLO_EL1,
+ APIBKEYHI_EL1,
+ APDAKEYLO_EL1,
+ APDAKEYHI_EL1,
+ APDBKEYLO_EL1,
+ APDBKEYHI_EL1,
+ APGAKEYLO_EL1,
+ APGAKEYHI_EL1,
+
/* 32bit specific registers. Keep them at the end of the range */
DACR32_EL2, /* Domain Access Control Register */
IFSR32_EL2, /* Instruction Fault Status Register */
@@ -439,6 +451,18 @@ static inline bool kvm_arch_requires_vhe(void)
return false;
}
+void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
+void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
+
+static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
+{
+ /* Disable ptrauth and use it in a lazy context via traps */
+ if (has_vhe() && kvm_supports_ptrauth())
+ kvm_arm_vcpu_ptrauth_disable(vcpu);
+}
+
+void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
+
static inline void kvm_arch_hardware_unsetup(void) {}
static inline void kvm_arch_sync_events(struct kvm *kvm) {}
static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 6e65cad..e559836 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -153,6 +153,13 @@ bool __fpsimd_enabled(void);
void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu);
+void __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt);
+void __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt);
+
u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
void __noreturn __hyp_do_panic(unsigned long, ...);
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4e2fb87..5cac605 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -749,6 +749,7 @@ static const char *esr_class_str[] = {
[ESR_ELx_EC_CP14_LS] = "CP14 LDC/STC",
[ESR_ELx_EC_FP_ASIMD] = "ASIMD",
[ESR_ELx_EC_CP10_ID] = "CP10 MRC/VMRS",
+ [ESR_ELx_EC_PAC] = "Pointer authentication trap",
[ESR_ELx_EC_CP14_64] = "CP14 MCRR/MRRC",
[ESR_ELx_EC_ILL] = "PSTATE.IL",
[ESR_ELx_EC_SVC32] = "SVC (AArch32)",
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 0b79834..5b980e7 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -174,19 +174,24 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
}
/*
+ * Handle the guest trying to use a ptrauth instruction, or trying to access a
+ * ptrauth register.
+ */
+void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
+{
+ if (has_vhe() && kvm_supports_ptrauth())
+ kvm_arm_vcpu_ptrauth_enable(vcpu);
+ else
+ kvm_inject_undefined(vcpu);
+}
+
+/*
* Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
- * a NOP).
+ * a NOP), or guest EL1 access to a ptrauth register.
*/
static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
- /*
- * We don't currently support ptrauth in a guest, and we mask the ID
- * registers to prevent well-behaved guests from trying to make use of
- * it.
- *
- * Inject an UNDEF, as if the feature really isn't present.
- */
- kvm_inject_undefined(vcpu);
+ kvm_arm_vcpu_ptrauth_trap(vcpu);
return 1;
}
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 82d1904..17cec99 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_KVM_ARM_HOST) += switch.o
obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
+obj-$(CONFIG_KVM_ARM_HOST) += ptrauth-sr.o
# KVM code is run at a different exception code with a different map, so
# compiler instrumentation that inserts callbacks or checks into the code may
diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
new file mode 100644
index 0000000..0576c01
--- /dev/null
+++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
+ *
+ * Copyright 2018 Arm Limited
+ * Author: Mark Rutland <mark.rutland@arm.com>
+ * Amit Daniel Kachhap <amit.kachhap@arm.com>
+ */
+#include <linux/compiler.h>
+#include <linux/kvm_host.h>
+
+#include <asm/cpucaps.h>
+#include <asm/cpufeature.h>
+#include <asm/kvm_asm.h>
+#include <asm/kvm_hyp.h>
+#include <asm/pointer_auth.h>
+
+static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
+{
+ return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
+ vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
+}
+
+void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
+ ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
+
+void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+ ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 03b36f1..301d332 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
sysreg_restore_guest_state_vhe(guest_ctxt);
__debug_switch_to_guest(vcpu);
+ __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
+
__set_guest_arch_workaround_state(vcpu);
do {
@@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
__set_host_arch_workaround_state(vcpu);
+ __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
+
sysreg_save_guest_state_vhe(guest_ctxt);
__deactivate_traps(vcpu);
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e3e3722..2546a65 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -986,6 +986,32 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
{ SYS_DESC(SYS_PMEVTYPERn_EL0(n)), \
access_pmu_evtyper, reset_unknown, (PMEVTYPER0_EL0 + n), }
+
+void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.ctxt.hcr_el2 |= (HCR_API | HCR_APK);
+}
+
+void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.ctxt.hcr_el2 &= ~(HCR_API | HCR_APK);
+}
+
+static bool trap_ptrauth(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *p,
+ const struct sys_reg_desc *rd)
+{
+ kvm_arm_vcpu_ptrauth_trap(vcpu);
+ return false;
+}
+
+#define __PTRAUTH_KEY(k) \
+ { SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k }
+
+#define PTRAUTH_KEY(k) \
+ __PTRAUTH_KEY(k ## KEYLO_EL1), \
+ __PTRAUTH_KEY(k ## KEYHI_EL1)
+
static bool access_cntp_tval(struct kvm_vcpu *vcpu,
struct sys_reg_params *p,
const struct sys_reg_desc *r)
@@ -1040,14 +1066,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
kvm_debug("SVE unsupported for guests, suppressing\n");
val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
- } else if (id == SYS_ID_AA64ISAR1_EL1) {
- const u64 ptrauth_mask = (0xfUL << ID_AA64ISAR1_APA_SHIFT) |
- (0xfUL << ID_AA64ISAR1_API_SHIFT) |
- (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
- (0xfUL << ID_AA64ISAR1_GPI_SHIFT);
- if (val & ptrauth_mask)
- kvm_debug("ptrauth unsupported for guests, suppressing\n");
- val &= ~ptrauth_mask;
} else if (id == SYS_ID_AA64MMFR1_EL1) {
if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
kvm_debug("LORegions unsupported for guests, suppressing\n");
@@ -1316,6 +1334,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
+ PTRAUTH_KEY(APIA),
+ PTRAUTH_KEY(APIB),
+ PTRAUTH_KEY(APDA),
+ PTRAUTH_KEY(APDB),
+ PTRAUTH_KEY(APGA),
+
{ SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
{ SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
{ SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 2d65ada..6d377d3 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -388,6 +388,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
vcpu_clear_wfe_traps(vcpu);
else
vcpu_set_wfe_traps(vcpu);
+
+ kvm_arm_vcpu_ptrauth_reset(vcpu);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
` (2 preceding siblings ...)
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-01-28 14:35 ` Julien Thierry
` (2 more replies)
2019-01-28 6:58 ` [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers Amit Daniel Kachhap
2019-01-28 6:58 ` [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
5 siblings, 3 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
This feature will allow the KVM guest to allow the handling of
pointer authentication instructions or to treat them as undefined
if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
supply this parameter instead of creating a new API.
A new register is not created to pass this parameter via
SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
supplied is enough to enable this feature.
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
Documentation/arm64/pointer-authentication.txt | 9 +++++----
Documentation/virtual/kvm/api.txt | 4 ++++
arch/arm/include/asm/kvm_host.h | 4 ++++
arch/arm64/include/asm/kvm_host.h | 7 ++++---
arch/arm64/include/uapi/asm/kvm.h | 1 +
arch/arm64/kvm/handle_exit.c | 3 ++-
arch/arm64/kvm/hyp/ptrauth-sr.c | 13 +++++++++++++
arch/arm64/kvm/reset.c | 3 +++
include/uapi/linux/kvm.h | 1 +
9 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
index a25cd21..0529a7d 100644
--- a/Documentation/arm64/pointer-authentication.txt
+++ b/Documentation/arm64/pointer-authentication.txt
@@ -82,7 +82,8 @@ pointers).
Virtualization
--------------
-Pointer authentication is not currently supported in KVM guests. KVM
-will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of
-the feature will result in an UNDEFINED exception being injected into
-the guest.
+Pointer authentication is enabled in KVM guest when virtual machine is
+created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
+to be enabled. Without this flag, pointer authentication is not enabled
+in KVM guests and attempted use of the feature will result in an UNDEFINED
+exception being injected into the guest.
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 356156f..1e646fb 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2642,6 +2642,10 @@ Possible features:
Depends on KVM_CAP_ARM_PSCI_0_2.
- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
Depends on KVM_CAP_ARM_PMU_V3.
+ - KVM_ARM_VCPU_PTRAUTH: Emulate Pointer authentication for the CPU.
+ Depends on KVM_CAP_ARM_PTRAUTH and only on arm64 architecture. If
+ set, then the KVM guest allows the execution of pointer authentication
+ instructions or treats them as undefined if not set.
4.83 KVM_ARM_PREFERRED_TARGET
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index b200c14..b6950df 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -346,6 +346,10 @@ static inline int kvm_arm_have_ssbd(void)
static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
+{
+ return false;
+}
#define __KVM_HAVE_ARCH_VM_ALLOC
struct kvm *kvm_arch_alloc_vm(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c798d0c..4a6ec40 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -43,7 +43,7 @@
#define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
-#define KVM_VCPU_MAX_FEATURES 4
+#define KVM_VCPU_MAX_FEATURES 5
#define KVM_REQ_SLEEP \
KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
@@ -453,14 +453,15 @@ static inline bool kvm_arch_requires_vhe(void)
void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
+bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu);
static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
{
/* Disable ptrauth and use it in a lazy context via traps */
- if (has_vhe() && kvm_supports_ptrauth())
+ if (has_vhe() && kvm_supports_ptrauth()
+ && kvm_arm_vcpu_ptrauth_allowed(vcpu))
kvm_arm_vcpu_ptrauth_disable(vcpu);
}
-
void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
static inline void kvm_arch_hardware_unsetup(void) {}
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 97c3478..5f82ca1 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -102,6 +102,7 @@ struct kvm_regs {
#define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */
#define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
#define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
+#define KVM_ARM_VCPU_PTRAUTH 4 /* VCPU uses address authentication */
struct kvm_vcpu_init {
__u32 target;
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 5b980e7..c0e5dcd 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -179,7 +179,8 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
*/
void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
{
- if (has_vhe() && kvm_supports_ptrauth())
+ if (has_vhe() && kvm_supports_ptrauth()
+ && kvm_arm_vcpu_ptrauth_allowed(vcpu))
kvm_arm_vcpu_ptrauth_enable(vcpu);
else
kvm_inject_undefined(vcpu);
diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
index 0576c01..369624f 100644
--- a/arch/arm64/kvm/hyp/ptrauth-sr.c
+++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
@@ -42,3 +42,16 @@ void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
}
+
+/**
+ * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
+ *
+ * @vcpu: The VCPU pointer
+ *
+ * This function will be used to enable/disable ptrauth in guest as configured
+ * by the KVM userspace API.
+ */
+bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
+{
+ return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
+}
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index b72a3dd..987e0c3c 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -91,6 +91,9 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_ARM_VM_IPA_SIZE:
r = kvm_ipa_limit;
break;
+ case KVM_CAP_ARM_PTRAUTH:
+ r = kvm_supports_ptrauth();
+ break;
default:
r = 0;
}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 6d4ea4b..a553477 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -988,6 +988,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_VM_IPA_SIZE 165
#define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
#define KVM_CAP_HYPERV_CPUID 167
+#define KVM_CAP_ARM_PTRAUTH 168
#ifdef KVM_CAP_IRQ_ROUTING
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
` (3 preceding siblings ...)
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-02-13 17:35 ` Kristina Martsenko
2019-01-28 6:58 ` [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
5 siblings, 1 reply; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
According to userspace settings, ptrauth key registers are conditionally
present in guest system register list based on user specified flag
KVM_ARM_VCPU_PTRAUTH.
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
Documentation/arm64/pointer-authentication.txt | 3 ++
arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++-------
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
index 0529a7d..3be4ee1 100644
--- a/Documentation/arm64/pointer-authentication.txt
+++ b/Documentation/arm64/pointer-authentication.txt
@@ -87,3 +87,6 @@ created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
to be enabled. Without this flag, pointer authentication is not enabled
in KVM guests and attempted use of the feature will result in an UNDEFINED
exception being injected into the guest.
+
+Additionally, when KVM_ARM_VCPU_PTRAUTH is not set then KVM will filter
+out the authentication key registers from userspace.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2546a65..b46a78e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1334,12 +1334,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
- PTRAUTH_KEY(APIA),
- PTRAUTH_KEY(APIB),
- PTRAUTH_KEY(APDA),
- PTRAUTH_KEY(APDB),
- PTRAUTH_KEY(APGA),
-
{ SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
{ SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
{ SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
@@ -1491,6 +1485,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x70 },
};
+static const struct sys_reg_desc ptrauth_reg_descs[] = {
+ PTRAUTH_KEY(APIA),
+ PTRAUTH_KEY(APIB),
+ PTRAUTH_KEY(APDA),
+ PTRAUTH_KEY(APDB),
+ PTRAUTH_KEY(APGA),
+};
+
static bool trap_dbgidr(struct kvm_vcpu *vcpu,
struct sys_reg_params *p,
const struct sys_reg_desc *r)
@@ -2093,6 +2095,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
r = find_reg(params, table, num);
if (!r)
r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+ if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
+ r = find_reg(params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
if (likely(r)) {
perform_access(vcpu, params, r);
@@ -2206,6 +2210,8 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
r = find_reg_by_id(id, ¶ms, table, num);
if (!r)
r = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+ if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
+ r = find_reg(¶ms, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
/* Not saved in the sys_reg array and not otherwise accessible? */
if (r && !(r->reg || r->get_user))
@@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
}
/* Assumed ordered tables, see kvm_sys_reg_table_init. */
-static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
+static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
+ const struct sys_reg_desc *desc, unsigned int len)
{
const struct sys_reg_desc *i1, *i2, *end1, *end2;
unsigned int total = 0;
size_t num;
int err;
+ if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
+ return total;
+
/* We check for duplicates here, to allow arch-specific overrides. */
i1 = get_target_table(vcpu->arch.target, true, &num);
end1 = i1 + num;
- i2 = sys_reg_descs;
- end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
+ i2 = desc;
+ end2 = desc + len;
BUG_ON(i1 == end1 || i2 == end2);
@@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
{
return ARRAY_SIZE(invariant_sys_regs)
+ num_demux_regs()
- + walk_sys_regs(vcpu, (u64 __user *)NULL);
+ + walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
+ ARRAY_SIZE(sys_reg_descs))
+ + walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
+ ARRAY_SIZE(ptrauth_reg_descs));
}
int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
@@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
uindices++;
}
- err = walk_sys_regs(vcpu, uindices);
+ err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+ if (err < 0)
+ return err;
+ uindices += err;
+
+ err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
if (err < 0)
return err;
uindices += err;
@@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
+ BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
/* We abuse the reset function to overwrite the table itself. */
for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
@@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
/* Generic chip reset first (so target could override). */
reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+ reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
table = get_target_table(vcpu->arch.target, true, &num);
reset_sys_reg_descs(vcpu, table, num);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
` (4 preceding siblings ...)
2019-01-28 6:58 ` [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers Amit Daniel Kachhap
@ 2019-01-28 6:58 ` Amit Daniel Kachhap
2019-01-28 14:56 ` Julien Thierry
5 siblings, 1 reply; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-01-28 6:58 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Amit Daniel Kachhap,
Mark Rutland
This is a runtime feature and can be enabled by --ptrauth option.
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arm/aarch32/include/kvm/kvm-cpu-arch.h | 2 ++
arm/aarch64/include/asm/kvm.h | 3 +++
arm/aarch64/include/kvm/kvm-arch.h | 1 +
arm/aarch64/include/kvm/kvm-config-arch.h | 4 +++-
arm/aarch64/include/kvm/kvm-cpu-arch.h | 2 ++
arm/aarch64/kvm-cpu.c | 5 +++++
arm/include/arm-common/kvm-config-arch.h | 1 +
arm/kvm-cpu.c | 7 +++++++
include/linux/kvm.h | 1 +
9 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
index d28ea67..5779767 100644
--- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
+++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
@@ -13,4 +13,6 @@
#define ARM_CPU_ID 0, 0, 0
#define ARM_CPU_ID_MPIDR 5
+unsigned int kvm__cpu_ptrauth_get_feature(void) {}
+
#endif /* KVM__KVM_CPU_ARCH_H */
diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
index c286035..0fd183d 100644
--- a/arm/aarch64/include/asm/kvm.h
+++ b/arm/aarch64/include/asm/kvm.h
@@ -98,6 +98,9 @@ struct kvm_regs {
#define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
#define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
+/* CPU uses address authentication and A key */
+#define KVM_ARM_VCPU_PTRAUTH 4
+
struct kvm_vcpu_init {
__u32 target;
__u32 features[7];
diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 9de623a..bd566cb 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -11,4 +11,5 @@
#include "arm-common/kvm-arch.h"
+
#endif /* KVM__KVM_ARCH_H */
diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
index 04be43d..2074684 100644
--- a/arm/aarch64/include/kvm/kvm-config-arch.h
+++ b/arm/aarch64/include/kvm/kvm-config-arch.h
@@ -8,7 +8,9 @@
"Create PMUv3 device"), \
OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \
"Specify random seed for Kernel Address Space " \
- "Layout Randomization (KASLR)"),
+ "Layout Randomization (KASLR)"), \
+ OPT_BOOLEAN('\0', "ptrauth", &(cfg)->has_ptrauth, \
+ "Enable address authentication"),
#include "arm-common/kvm-config-arch.h"
diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
index a9d8563..f7b64b7 100644
--- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
+++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
@@ -17,4 +17,6 @@
#define ARM_CPU_CTRL 3, 0, 1, 0
#define ARM_CPU_CTRL_SCTLR_EL1 0
+unsigned int kvm__cpu_ptrauth_get_feature(void);
+
#endif /* KVM__KVM_CPU_ARCH_H */
diff --git a/arm/aarch64/kvm-cpu.c b/arm/aarch64/kvm-cpu.c
index 1b29374..10da2cb 100644
--- a/arm/aarch64/kvm-cpu.c
+++ b/arm/aarch64/kvm-cpu.c
@@ -123,6 +123,11 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu)
return reset_vcpu_aarch64(vcpu);
}
+unsigned int kvm__cpu_ptrauth_get_feature(void)
+{
+ return (1UL << KVM_ARM_VCPU_PTRAUTH);
+}
+
int kvm_cpu__get_endianness(struct kvm_cpu *vcpu)
{
struct kvm_one_reg reg;
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index 6a196f1..eb872db 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -10,6 +10,7 @@ struct kvm_config_arch {
bool aarch32_guest;
bool has_pmuv3;
u64 kaslr_seed;
+ bool has_ptrauth;
enum irqchip_type irqchip;
};
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index 7780251..5afd727 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -68,6 +68,13 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2);
}
+ /* Set KVM_ARM_VCPU_PTRAUTH_I_A if available */
+ if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH)) {
+ if (kvm->cfg.arch.has_ptrauth)
+ vcpu_init.features[0] |=
+ kvm__cpu_ptrauth_get_feature();
+ }
+
/*
* If the preferred target ioctl is successful then
* use preferred target else try each and every target type
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index f51d508..204315e 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_PPC_MMU_RADIX 134
#define KVM_CAP_PPC_MMU_HASH_V3 135
#define KVM_CAP_IMMEDIATE_EXIT 136
+#define KVM_CAP_ARM_PTRAUTH 168
#ifdef KVM_CAP_IRQ_ROUTING
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
@ 2019-01-28 14:25 ` Julien Thierry
2019-01-31 16:25 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register James Morse
2019-02-13 17:34 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Kristina Martsenko
2 siblings, 0 replies; 31+ messages in thread
From: Julien Thierry @ 2019-01-28 14:25 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Marc Zyngier, Catalin Marinas, Will Deacon, Kristina Martsenko,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When pointer authentication is supported, a guest may wish to use it.
> This patch adds the necessary KVM infrastructure for this to work, with
> a semi-lazy context switch of the pointer auth state.
>
> Pointer authentication feature is only enabled when VHE is built
> into the kernel and present into CPU implementation so only VHE code
> paths are modified.
>
> When we schedule a vcpu, we disable guest usage of pointer
> authentication instructions and accesses to the keys. While these are
> disabled, we avoid context-switching the keys. When we trap the guest
> trying to use pointer authentication functionality, we change to eagerly
> context-switching the keys, and enable the feature. The next time the
> vcpu is scheduled out/in, we start again.
>
> Pointer authentication consists of address authentication and generic
> authentication, and CPUs in a system might have varied support for
> either. Where support for either feature is not uniform, it is hidden
> from guests via ID register emulation, as a result of the cpufeature
> framework in the host.
>
> Unfortunately, address authentication and generic authentication cannot
> be trapped separately, as the architecture provides a single EL2 trap
> covering both. If we wish to expose one without the other, we cannot
> prevent a (badly-written) guest from intermittently using a feature
> which is not uniformly supported (when scheduled on a physical CPU which
> supports the relevant feature). When the guest is scheduled on a
> physical CPU lacking the feature, these attempts will result in an UNDEF
> being taken by the guest.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> arch/arm/include/asm/kvm_host.h | 1 +
> arch/arm64/include/asm/cpufeature.h | 5 +++++
> arch/arm64/include/asm/kvm_host.h | 24 ++++++++++++++++++++
> arch/arm64/include/asm/kvm_hyp.h | 7 ++++++
> arch/arm64/kernel/traps.c | 1 +
> arch/arm64/kvm/handle_exit.c | 23 +++++++++++--------
> arch/arm64/kvm/hyp/Makefile | 1 +
> arch/arm64/kvm/hyp/ptrauth-sr.c | 44 +++++++++++++++++++++++++++++++++++++
> arch/arm64/kvm/hyp/switch.c | 4 ++++
> arch/arm64/kvm/sys_regs.c | 40 ++++++++++++++++++++++++++-------
> virt/kvm/arm/arm.c | 2 ++
> 11 files changed, 135 insertions(+), 17 deletions(-)
> create mode 100644 arch/arm64/kvm/hyp/ptrauth-sr.c
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 704667e..b200c14 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -345,6 +345,7 @@ static inline int kvm_arm_have_ssbd(void)
>
> static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
> +static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
>
> #define __KVM_HAVE_ARCH_VM_ALLOC
> struct kvm *kvm_arch_alloc_vm(void);
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index dfcfba7..e1bf2a5 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
> cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
> }
>
> +static inline bool kvm_supports_ptrauth(void)
> +{
> + return system_supports_address_auth() && system_supports_generic_auth();
> +}
> +
> #define ARM64_SSBD_UNKNOWN -1
> #define ARM64_SSBD_FORCE_DISABLE 0
> #define ARM64_SSBD_KERNEL 1
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1f2d237..c798d0c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -146,6 +146,18 @@ enum vcpu_sysreg {
> PMSWINC_EL0, /* Software Increment Register */
> PMUSERENR_EL0, /* User Enable Register */
>
> + /* Pointer Authentication Registers */
> + APIAKEYLO_EL1,
> + APIAKEYHI_EL1,
> + APIBKEYLO_EL1,
> + APIBKEYHI_EL1,
> + APDAKEYLO_EL1,
> + APDAKEYHI_EL1,
> + APDBKEYLO_EL1,
> + APDBKEYHI_EL1,
> + APGAKEYLO_EL1,
> + APGAKEYHI_EL1,
> +
> /* 32bit specific registers. Keep them at the end of the range */
> DACR32_EL2, /* Domain Access Control Register */
> IFSR32_EL2, /* Instruction Fault Status Register */
> @@ -439,6 +451,18 @@ static inline bool kvm_arch_requires_vhe(void)
> return false;
> }
>
> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
> +
> +static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
> +{
> + /* Disable ptrauth and use it in a lazy context via traps */
> + if (has_vhe() && kvm_supports_ptrauth())
Since we state that our support of ptrauth for kvm guests depends on
vhe, maybe it would make sense to put has_vhe() inside
kvm_supports_ptrauth(). This way the dependency is explicitly expressed
in the code.
> + kvm_arm_vcpu_ptrauth_disable(vcpu);
> +}
> +
> +void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
> +
> static inline void kvm_arch_hardware_unsetup(void) {}
> static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 6e65cad..e559836 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -153,6 +153,13 @@ bool __fpsimd_enabled(void);
> void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu);
>
> +void __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt);
> +void __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt);
> +
> u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
> void __noreturn __hyp_do_panic(unsigned long, ...);
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 4e2fb87..5cac605 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -749,6 +749,7 @@ static const char *esr_class_str[] = {
> [ESR_ELx_EC_CP14_LS] = "CP14 LDC/STC",
> [ESR_ELx_EC_FP_ASIMD] = "ASIMD",
> [ESR_ELx_EC_CP10_ID] = "CP10 MRC/VMRS",
> + [ESR_ELx_EC_PAC] = "Pointer authentication trap",
> [ESR_ELx_EC_CP14_64] = "CP14 MCRR/MRRC",
> [ESR_ELx_EC_ILL] = "PSTATE.IL",
> [ESR_ELx_EC_SVC32] = "SVC (AArch32)",
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 0b79834..5b980e7 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -174,19 +174,24 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
> }
>
> /*
> + * Handle the guest trying to use a ptrauth instruction, or trying to access a
> + * ptrauth register.
> + */
> +void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
Nit: I was wondering whether it would make more sense as static inline
in the same header as "kvm_arm_vcpu_ptrauth_reset()"
> +{
> + if (has_vhe() && kvm_supports_ptrauth())> + kvm_arm_vcpu_ptrauth_enable(vcpu);
> + else
> + kvm_inject_undefined(vcpu);
> +}
> +
> +/*
> * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
> - * a NOP).
> + * a NOP), or guest EL1 access to a ptrauth register.
Nit: This comment becomes a bit redundant with the one above, don't know
whether that's a bad thing.
Otherwise things look good to me:
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> */
> static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> - /*
> - * We don't currently support ptrauth in a guest, and we mask the ID
> - * registers to prevent well-behaved guests from trying to make use of
> - * it.
> - *
> - * Inject an UNDEF, as if the feature really isn't present.
> - */
> - kvm_inject_undefined(vcpu);
> + kvm_arm_vcpu_ptrauth_trap(vcpu);
> return 1;
> }
>
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 82d1904..17cec99 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_KVM_ARM_HOST) += switch.o
> obj-$(CONFIG_KVM_ARM_HOST) += fpsimd.o
> obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
> obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
> +obj-$(CONFIG_KVM_ARM_HOST) += ptrauth-sr.o
>
> # KVM code is run at a different exception code with a different map, so
> # compiler instrumentation that inserts callbacks or checks into the code may
> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
> new file mode 100644
> index 0000000..0576c01
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
> + *
> + * Copyright 2018 Arm Limited
> + * Author: Mark Rutland <mark.rutland@arm.com>
> + * Amit Daniel Kachhap <amit.kachhap@arm.com>
> + */
> +#include <linux/compiler.h>
> +#include <linux/kvm_host.h>
> +
> +#include <asm/cpucaps.h>
> +#include <asm/cpufeature.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_hyp.h>
> +#include <asm/pointer_auth.h>
> +
> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
> +{
> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> + ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 03b36f1..301d332 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> sysreg_restore_guest_state_vhe(guest_ctxt);
> __debug_switch_to_guest(vcpu);
>
> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
> +
> __set_guest_arch_workaround_state(vcpu);
>
> do {
> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>
> __set_host_arch_workaround_state(vcpu);
>
> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
> +
> sysreg_save_guest_state_vhe(guest_ctxt);
>
> __deactivate_traps(vcpu);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e3e3722..2546a65 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -986,6 +986,32 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> { SYS_DESC(SYS_PMEVTYPERn_EL0(n)), \
> access_pmu_evtyper, reset_unknown, (PMEVTYPER0_EL0 + n), }
>
> +
> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
> +{
> + vcpu->arch.ctxt.hcr_el2 |= (HCR_API | HCR_APK);
> +}
> +
> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
> +{
> + vcpu->arch.ctxt.hcr_el2 &= ~(HCR_API | HCR_APK);
> +}
> +
> +static bool trap_ptrauth(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *rd)
> +{
> + kvm_arm_vcpu_ptrauth_trap(vcpu);
> + return false;
> +}
> +
> +#define __PTRAUTH_KEY(k) \
> + { SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k }
> +
> +#define PTRAUTH_KEY(k) \
> + __PTRAUTH_KEY(k ## KEYLO_EL1), \
> + __PTRAUTH_KEY(k ## KEYHI_EL1)
> +
> static bool access_cntp_tval(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> @@ -1040,14 +1066,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
> kvm_debug("SVE unsupported for guests, suppressing\n");
>
> val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
> - } else if (id == SYS_ID_AA64ISAR1_EL1) {
> - const u64 ptrauth_mask = (0xfUL << ID_AA64ISAR1_APA_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_API_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_GPI_SHIFT);
> - if (val & ptrauth_mask)
> - kvm_debug("ptrauth unsupported for guests, suppressing\n");
> - val &= ~ptrauth_mask;
> } else if (id == SYS_ID_AA64MMFR1_EL1) {
> if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> kvm_debug("LORegions unsupported for guests, suppressing\n");
> @@ -1316,6 +1334,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
> { SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
>
> + PTRAUTH_KEY(APIA),
> + PTRAUTH_KEY(APIB),
> + PTRAUTH_KEY(APDA),
> + PTRAUTH_KEY(APDB),
> + PTRAUTH_KEY(APGA),
> +
> { SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
> { SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
> { SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 2d65ada..6d377d3 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -388,6 +388,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> vcpu_clear_wfe_traps(vcpu);
> else
> vcpu_set_wfe_traps(vcpu);
> +
> + kvm_arm_vcpu_ptrauth_reset(vcpu);
> }
>
> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>
--
Julien Thierry
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
@ 2019-01-28 14:35 ` Julien Thierry
2019-01-31 16:27 ` James Morse
2019-02-13 17:35 ` Kristina Martsenko
2 siblings, 0 replies; 31+ messages in thread
From: Julien Thierry @ 2019-01-28 14:35 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Marc Zyngier, Catalin Marinas, Will Deacon, Kristina Martsenko,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> This feature will allow the KVM guest to allow the handling of
> pointer authentication instructions or to treat them as undefined
> if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
> supply this parameter instead of creating a new API.
>
> A new register is not created to pass this parameter via
> SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
> supplied is enough to enable this feature.
>
I had a bit of trouble parsing this last sentence. I'd suggest rewording
it as "No additional register is created for the SET/GET_ONE_REG API,
instead just pass this parameter as a flag".
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> Documentation/arm64/pointer-authentication.txt | 9 +++++----
> Documentation/virtual/kvm/api.txt | 4 ++++
> arch/arm/include/asm/kvm_host.h | 4 ++++
> arch/arm64/include/asm/kvm_host.h | 7 ++++---
> arch/arm64/include/uapi/asm/kvm.h | 1 +
> arch/arm64/kvm/handle_exit.c | 3 ++-
> arch/arm64/kvm/hyp/ptrauth-sr.c | 13 +++++++++++++
> arch/arm64/kvm/reset.c | 3 +++
> include/uapi/linux/kvm.h | 1 +
> 9 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
> index a25cd21..0529a7d 100644
> --- a/Documentation/arm64/pointer-authentication.txt
> +++ b/Documentation/arm64/pointer-authentication.txt
> @@ -82,7 +82,8 @@ pointers).
> Virtualization
> --------------
>
> -Pointer authentication is not currently supported in KVM guests. KVM
> -will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of
> -the feature will result in an UNDEFINED exception being injected into
> -the guest.
> +Pointer authentication is enabled in KVM guest when virtual machine is
> +created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
> +to be enabled. Without this flag, pointer authentication is not enabled
> +in KVM guests and attempted use of the feature will result in an UNDEFINED
> +exception being injected into the guest.
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 356156f..1e646fb 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2642,6 +2642,10 @@ Possible features:
> Depends on KVM_CAP_ARM_PSCI_0_2.
> - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
> Depends on KVM_CAP_ARM_PMU_V3.
> + - KVM_ARM_VCPU_PTRAUTH: Emulate Pointer authentication for the CPU.
> + Depends on KVM_CAP_ARM_PTRAUTH and only on arm64 architecture. If
> + set, then the KVM guest allows the execution of pointer authentication
> + instructions or treats them as undefined if not set.
I'd suggest "then the KVM guest allows the execution of pointer
authentication instructions. Otherwise, KVM treats these instructions as
undefined."
>
>
> 4.83 KVM_ARM_PREFERRED_TARGET
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index b200c14..b6950df 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -346,6 +346,10 @@ static inline int kvm_arm_have_ssbd(void)
> static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
> +static inline bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
>
> #define __KVM_HAVE_ARCH_VM_ALLOC
> struct kvm *kvm_arch_alloc_vm(void);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c798d0c..4a6ec40 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -43,7 +43,7 @@
>
> #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>
> -#define KVM_VCPU_MAX_FEATURES 4
> +#define KVM_VCPU_MAX_FEATURES 5
>
> #define KVM_REQ_SLEEP \
> KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> @@ -453,14 +453,15 @@ static inline bool kvm_arch_requires_vhe(void)
>
> void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
> void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu);
>
> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
> {
> /* Disable ptrauth and use it in a lazy context via traps */
> - if (has_vhe() && kvm_supports_ptrauth())
> + if (has_vhe() && kvm_supports_ptrauth()
> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> kvm_arm_vcpu_ptrauth_disable(vcpu);
> }
> -
> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
>
> static inline void kvm_arch_hardware_unsetup(void) {}
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 97c3478..5f82ca1 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -102,6 +102,7 @@ struct kvm_regs {
> #define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */
> #define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
> #define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
> +#define KVM_ARM_VCPU_PTRAUTH 4 /* VCPU uses address authentication */
>
> struct kvm_vcpu_init {
> __u32 target;
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 5b980e7..c0e5dcd 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -179,7 +179,8 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
> */
> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
> {
> - if (has_vhe() && kvm_supports_ptrauth())
> + if (has_vhe() && kvm_supports_ptrauth()
> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> kvm_arm_vcpu_ptrauth_enable(vcpu);
> else
> kvm_inject_undefined(vcpu);
> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
> index 0576c01..369624f 100644
> --- a/arch/arm64/kvm/hyp/ptrauth-sr.c
> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
> @@ -42,3 +42,16 @@ void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
> ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> }
> +
> +/**
> + * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
> + *
> + * @vcpu: The VCPU pointer
> + *
> + * This function will be used to enable/disable ptrauth in guest as configured
> + * by the KVM userspace API.
> + */
> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
Nit: I find the "allowed" a bit strange. Maybe something like
"kvm_arm_vcpu_has_ptrauth()" ?
> +{
> + return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
> +}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index b72a3dd..987e0c3c 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -91,6 +91,9 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_ARM_VM_IPA_SIZE:
> r = kvm_ipa_limit;
> break;
> + case KVM_CAP_ARM_PTRAUTH:
> + r = kvm_supports_ptrauth();
Even if we don't have VHE?
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication
2019-01-28 6:58 ` [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
@ 2019-01-28 14:56 ` Julien Thierry
0 siblings, 0 replies; 31+ messages in thread
From: Julien Thierry @ 2019-01-28 14:56 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
Kristina Martsenko, linux-kernel, Mark Rutland
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> This is a runtime feature and can be enabled by --ptrauth option.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> arm/aarch32/include/kvm/kvm-cpu-arch.h | 2 ++
> arm/aarch64/include/asm/kvm.h | 3 +++
> arm/aarch64/include/kvm/kvm-arch.h | 1 +
> arm/aarch64/include/kvm/kvm-config-arch.h | 4 +++-
> arm/aarch64/include/kvm/kvm-cpu-arch.h | 2 ++
> arm/aarch64/kvm-cpu.c | 5 +++++
> arm/include/arm-common/kvm-config-arch.h | 1 +
> arm/kvm-cpu.c | 7 +++++++
> include/linux/kvm.h | 1 +
> 9 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> index d28ea67..5779767 100644
> --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> @@ -13,4 +13,6 @@
> #define ARM_CPU_ID 0, 0, 0
> #define ARM_CPU_ID_MPIDR 5
>
> +unsigned int kvm__cpu_ptrauth_get_feature(void) {}
> +
You probably want a return statement for this function.
> #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
> index c286035..0fd183d 100644
> --- a/arm/aarch64/include/asm/kvm.h
> +++ b/arm/aarch64/include/asm/kvm.h
> @@ -98,6 +98,9 @@ struct kvm_regs {
> #define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
> #define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
>
> +/* CPU uses address authentication and A key */
> +#define KVM_ARM_VCPU_PTRAUTH 4
> +
> struct kvm_vcpu_init {
> __u32 target;
> __u32 features[7];
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 9de623a..bd566cb 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -11,4 +11,5 @@
>
> #include "arm-common/kvm-arch.h"
>
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
> index 04be43d..2074684 100644
> --- a/arm/aarch64/include/kvm/kvm-config-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-config-arch.h
> @@ -8,7 +8,9 @@
> "Create PMUv3 device"), \
> OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \
> "Specify random seed for Kernel Address Space " \
> - "Layout Randomization (KASLR)"),
> + "Layout Randomization (KASLR)"), \
> + OPT_BOOLEAN('\0', "ptrauth", &(cfg)->has_ptrauth, \
> + "Enable address authentication"),
>
> #include "arm-common/kvm-config-arch.h"
>
> diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> index a9d8563..f7b64b7 100644
> --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h
> @@ -17,4 +17,6 @@
> #define ARM_CPU_CTRL 3, 0, 1, 0
> #define ARM_CPU_CTRL_SCTLR_EL1 0
>
> +unsigned int kvm__cpu_ptrauth_get_feature(void);
> +> #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/aarch64/kvm-cpu.c b/arm/aarch64/kvm-cpu.c
> index 1b29374..10da2cb 100644
> --- a/arm/aarch64/kvm-cpu.c
> +++ b/arm/aarch64/kvm-cpu.c
> @@ -123,6 +123,11 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu)
> return reset_vcpu_aarch64(vcpu);
> }
>
> +unsigned int kvm__cpu_ptrauth_get_feature(void)
> +{
> + return (1UL << KVM_ARM_VCPU_PTRAUTH);
> +}
> +
Couldn't this be a simple:
#define PTRAUTH_FEATURE (1UL << KVM_ARM_VCPU_PTRAUTH)
And just define it to 0 for the aarch32 side?
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys
2019-01-28 6:58 ` [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys Amit Daniel Kachhap
@ 2019-01-31 16:20 ` James Morse
2019-02-13 17:32 ` Kristina Martsenko
0 siblings, 1 reply; 31+ messages in thread
From: James Morse @ 2019-01-31 16:20 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> The keys can be switched either inside an assembly or such
> functions which do not have pointer authentication checks, so a GCC
> attribute is added to enable it.
>
> A function ptrauth_keys_store is added which is similar to existing
> function ptrauth_keys_switch but saves the key values in memory.
> This may be useful for save/restore scenarios when CPU changes
> privilege levels, suspend/resume etc.
> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index 15d4951..98441ce 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -11,6 +11,13 @@
>
> #ifdef CONFIG_ARM64_PTR_AUTH
> /*
> + * Compile the function without pointer authentication instructions. This
> + * allows pointer authentication to be enabled/disabled within the function
> + * (but leaves the function unprotected by pointer authentication).
> + */
> +#define __no_ptrauth __attribute__((target("sign-return-address=none")))
The documentation[0] for this says 'none' is the default. Will this only
take-effect once the kernel supports pointer-auth for the host? (Is this just
documentation until then?)
('noptrauth' would fit with 'notrace' slightly better)
Thanks,
James
[0]
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html#AArch64-Function-Attributes
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-01-28 6:58 ` [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value Amit Daniel Kachhap
@ 2019-01-31 16:22 ` James Morse
2019-02-14 9:49 ` Amit Daniel Kachhap
2019-02-13 17:34 ` Kristina Martsenko
1 sibling, 1 reply; 31+ messages in thread
From: James Morse @ 2019-01-31 16:22 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
> is a constant value. This works today, as the host HCR_EL2 value is
> always the same, but this will get in the way of supporting extensions
> that require HCR_EL2 bits to be set conditionally for the host.
>
> To allow such features to work without KVM having to explicitly handle
> every possible host feature combination, this patch has KVM save/restore
> the host HCR when switching to/from a guest HCR. The saving of the
> register is done once during cpu hypervisor initialization state and is
> just restored after switch from guest.
>
> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using
> kvm_call_hyp and is helpful in NHVE case.
> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated
> to toggle the TGE bit with a RMW sequence, as we already do in
> __tlb_switch_to_guest_vhe().
> While at it, host MDCR_EL2 value is fetched in a similar way and restored
> after every switch from host to guest. There should not be any change in
> functionality due to this.
Could this step be done as a separate subsequent patch? It would make review
easier! The MDCR stuff would be a simplification if done second, done in one go
like this its pretty noisy.
There ought to be some justification for moving hcr/mdcr into the cpu_context in
the commit message.
If you're keeping Mark's 'Signed-off-by' its would be normal to keep Mark as the
author in git. This shows up a an extra 'From:' when you post the patch, and
gets picked up when the maintainer runs git-am.
This patch has changed substantially from Mark's version:
https://lkml.org/lkml/2017/11/27/675
If you keep the signed-off-by, could you add a [note] in the signed-off area
with a terse summary. Something like:
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
[ Move hcr to cpu_context, added __cpu_copy_hyp_conf()]
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
(9c06602b1b92 is a good picked-at-random example for both of these)
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index f5b79e9..2da6e43 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void);
>
> extern u32 __kvm_get_mdcr_el2(void);
>
> +extern u64 __kvm_get_hcr_el2(void);
Do we need these in separate helpers? For non-vhe this means two separate trips
to EL2. Something like kvm_populate_host_context(void), and an __ version for
the bit at EL2?
We don't need to pass the host-context to EL2 as once kvm is loaded we can
access host per-cpu variables at EL2 using __hyp_this_cpu_read(). This will save
passing the vcpu around.
> @@ -458,6 +457,25 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>
> static inline void __cpu_init_stage2(void) {}
>
> +/**
> + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers
> + *
> + * It is called once per-cpu during CPU hyp initialisation.
> + */
> +static inline void __cpu_copy_hyp_conf(void)
> +{
> + kvm_cpu_context_t *host_cxt = this_cpu_ptr(&kvm_host_cpu_state);
> +
> + host_cxt->hcr_el2 = kvm_call_hyp(__kvm_get_hcr_el2);
> +
> + /*
> + * Retrieve the initial value of mdcr_el2 so we can preserve
> + * MDCR_EL2.HPMN which has presumably been set-up by some
> + * knowledgeable bootcode.
> + */
> + host_cxt->mdcr_el2 = kvm_call_hyp(__kvm_get_mdcr_el2);
> +}
Its strange to make this an inline in a header. kvm_arm_init_debug() is a
static-inline for arch/arm, but a regular C function for arch/arm64. Can't we do
the same?
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 68d6f7c..22c854a 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -316,3 +316,14 @@ void __hyp_text __kvm_enable_ssbs(void)
> "msr sctlr_el2, %0"
> : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS));
> }
> +
> +/**
> + * __read_hyp_hcr_el2 - Returns hcr_el2 register value
> + *
> + * This function acts as a function handler parameter for kvm_call_hyp and
> + * may be called from EL1 exception level to fetch the register value.
> + */
> +u64 __hyp_text __kvm_get_hcr_el2(void)
> +{
> + return read_sysreg(hcr_el2);
> +}
While I'm all in favour of kernel-doc comments for functions, it may be
over-kill in this case!
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 9e350fd3..2d65ada 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -1327,10 +1327,10 @@ static void cpu_hyp_reinit(void)
> else
> cpu_init_hyp_mode(NULL);
>
> - kvm_arm_init_debug();
> -
> if (vgic_present)
> kvm_vgic_init_cpu_hardware();
> +
> + __cpu_copy_hyp_conf();
> }
Was there a reason to make this call later than it originally was?
(kvm_vgic_init_cpu_hardware() doesn't use any of those values, so its fine, just
curious!)
Thanks,
James
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
2019-01-28 14:25 ` Julien Thierry
@ 2019-01-31 16:25 ` James Morse
2019-02-13 17:35 ` Kristina Martsenko
2019-02-14 10:16 ` Amit Daniel Kachhap
2019-02-13 17:34 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Kristina Martsenko
2 siblings, 2 replies; 31+ messages in thread
From: James Morse @ 2019-01-31 16:25 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When pointer authentication is supported, a guest may wish to use it.
> This patch adds the necessary KVM infrastructure for this to work, with
> a semi-lazy context switch of the pointer auth state.
>
> Pointer authentication feature is only enabled when VHE is built
> into the kernel and present into CPU implementation so only VHE code
~s/into/in the/?
> paths are modified.
>
> When we schedule a vcpu, we disable guest usage of pointer
> authentication instructions and accesses to the keys. While these are
> disabled, we avoid context-switching the keys. When we trap the guest
> trying to use pointer authentication functionality, we change to eagerly
> context-switching the keys, and enable the feature. The next time the
> vcpu is scheduled out/in, we start again.
> Pointer authentication consists of address authentication and generic
> authentication, and CPUs in a system might have varied support for
> either. Where support for either feature is not uniform, it is hidden
> from guests via ID register emulation, as a result of the cpufeature
> framework in the host.
> Unfortunately, address authentication and generic authentication cannot
> be trapped separately, as the architecture provides a single EL2 trap
> covering both. If we wish to expose one without the other, we cannot
> prevent a (badly-written) guest from intermittently using a feature
> which is not uniformly supported (when scheduled on a physical CPU which
> supports the relevant feature). When the guest is scheduled on a
> physical CPU lacking the feature, these attempts will result in an UNDEF
> being taken by the guest.
This won't be fun. Can't KVM check that both are supported on all CPUs to avoid
this? ...
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index dfcfba7..e1bf2a5 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
> cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
> }
>
> +static inline bool kvm_supports_ptrauth(void)
> +{
> + return system_supports_address_auth() && system_supports_generic_auth();
> +}
... oh you do check. Could you cover this in the commit message? (to avoid an
UNDEF being taken by the guest we ... )
cpufeature.h is a strange place to put this, there are no other kvm symbols in
there. But there are users of system_supports_foo() in kvm_host.h.
> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
> new file mode 100644
> index 0000000..0576c01
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
> + *
> + * Copyright 2018 Arm Limited
> + * Author: Mark Rutland <mark.rutland@arm.com>
> + * Amit Daniel Kachhap <amit.kachhap@arm.com>
> + */
> +#include <linux/compiler.h>
> +#include <linux/kvm_host.h>
> +
> +#include <asm/cpucaps.h>
> +#include <asm/cpufeature.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_hyp.h>
> +#include <asm/pointer_auth.h>
> +
> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
Why __always_inline? Doesn't the compiler decide for 'static' symbols in C files?
> +{
> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
We can't cast part of an array to a structure like this. What happens if the
compiler inserts padding in struct-ptrauth_keys, or the struct randomization
thing gets hold of it: https://lwn.net/Articles/722293/
If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
of the sys_reg array.
Wouldn't the host keys be available somewhere else? (they must get transfer to
secondary CPUs somehow). Can we skip the save step when switching from the host?
> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1f2d237..c798d0c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -439,6 +451,18 @@ static inline bool kvm_arch_requires_vhe(void)
> return false;
> }
>
> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
> +
> +static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
> +{
> + /* Disable ptrauth and use it in a lazy context via traps */
> + if (has_vhe() && kvm_supports_ptrauth())
> + kvm_arm_vcpu_ptrauth_disable(vcpu);
> +}
(Could you move 'has_vhe()' into kvm_supports_ptrauth()? It fits with testing
the other cpufeatures, and makes this a little more readable when you add
'allowed' to it later.)
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 03b36f1..301d332 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> sysreg_restore_guest_state_vhe(guest_ctxt);
> __debug_switch_to_guest(vcpu);
>
> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
> +
> __set_guest_arch_workaround_state(vcpu);
>
> do {
> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>
> __set_host_arch_workaround_state(vcpu);
>
> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
> +
> sysreg_save_guest_state_vhe(guest_ctxt);
>
> __deactivate_traps(vcpu);
...This makes me nervous...
__guest_enter() is a function that (might) change the keys, then we change them
again here. We can't have any signed return address between these two points. I
don't trust the compiler not to generate any.
~
I had a chat with some friendly compiler folk... because there are two identical
sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
move the common code to a function it then calls. Apparently this is called
'function outlining'.
If the compiler does this, and the guest changes the keys, I think we would fail
the return address check.
Painting the whole thing with no_prauth would solve this, but this code then
becomes a target.
Because the compiler can't anticipate the keys changing, we ought to treat them
the same way we do the callee saved registers, stack-pointer etc, and
save/restore them in the __guest_enter() assembly code.
(we can still keep the save/restore in C, but call it from assembly so we know
nothing new is going on the stack).
Thanks,
James
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
2019-01-28 14:35 ` Julien Thierry
@ 2019-01-31 16:27 ` James Morse
2019-02-14 10:47 ` Amit Daniel Kachhap
2019-02-13 17:35 ` Kristina Martsenko
2 siblings, 1 reply; 31+ messages in thread
From: James Morse @ 2019-01-31 16:27 UTC (permalink / raw)
To: Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> This feature will allow the KVM guest to allow the handling of
> pointer authentication instructions or to treat them as undefined
> if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
> supply this parameter instead of creating a new API.
>
> A new register is not created to pass this parameter via
> SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
> supplied is enough to enable this feature.
> diff --git a/Documentation/arm64/pointer-authentication.txt
b/Documentation/arm64/pointer-authentication.txt
> index a25cd21..0529a7d 100644
> --- a/Documentation/arm64/pointer-authentication.txt
> +++ b/Documentation/arm64/pointer-authentication.txt
> @@ -82,7 +82,8 @@ pointers).
> Virtualization
> --------------
>
> -Pointer authentication is not currently supported in KVM guests. KVM
> -will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of
> -the feature will result in an UNDEFINED exception being injected into
> -the guest.
> +Pointer authentication is enabled in KVM guest when virtual machine is
> +created by passing a flag (KVM_ARM_VCPU_PTRAUTH)
Isn't that a VCPU flag? Shouldn't this be when each VCPU is created?
> requesting this feature
> +to be enabled. Without this flag, pointer authentication is not enabled
> +in KVM guests and attempted use of the feature will result in an UNDEFINED
> +exception being injected into the guest.
... what happens if KVM's user-space enables ptrauth on some vcpus, but not on
others?
You removed the id-register suppression in the previous patch, but it doesn't
get hooked up to kvm_arm_vcpu_ptrauth_allowed() here. (you could add
kvm_arm_vcpu_ptrauth_allowed() earlier, and default it to true to make it easier).
Doesn't this mean that if the CPU supports pointer auth, but user-space doesn't
specify this flag, the guest gets mysterious undef's whenever it tries to use
the advertised feature?
(whether we support big/little virtual-machines is probably a separate issue,
but the id registers need to be consistent with our trap-and-undef behaviour)
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c798d0c..4a6ec40 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -453,14 +453,15 @@ static inline bool kvm_arch_requires_vhe(void)
>
> void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
> void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu);
>
> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
> {
> /* Disable ptrauth and use it in a lazy context via traps */
> - if (has_vhe() && kvm_supports_ptrauth())
> + if (has_vhe() && kvm_supports_ptrauth()
> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> kvm_arm_vcpu_ptrauth_disable(vcpu);
> }
> -
> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 5b980e7..c0e5dcd 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -179,7 +179,8 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
> */
> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
> {
> - if (has_vhe() && kvm_supports_ptrauth())
> + if (has_vhe() && kvm_supports_ptrauth()
> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
Duplication. If has_vhe() moved into kvm_supports_ptrauth(), and
kvm_supports_ptrauth() was called from kvm_arm_vcpu_ptrauth_allowed() it would
be clearer that use of this feature was becoming user-controlled policy.
(We don't need to list the dependencies at every call site)
> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
> index 0576c01..369624f 100644
> --- a/arch/arm64/kvm/hyp/ptrauth-sr.c
> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
> @@ -42,3 +42,16 @@ void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
> ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> }
> +
> +/**
> + * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
('enabled by KVM's user-space' may be clearer. 'Present in vcpu' could be down
to a cpufeature thing)
> + *
> + * @vcpu: The VCPU pointer
> + *
> + * This function will be used to enable/disable ptrauth in guest as configured
... but it just tests the bit ...
> + * by the KVM userspace API.
> + */
> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
> +{
> + return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
> +}
Thanks,
James
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys
2019-01-31 16:20 ` James Morse
@ 2019-02-13 17:32 ` Kristina Martsenko
2019-02-14 10:53 ` Amit Daniel Kachhap
0 siblings, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:32 UTC (permalink / raw)
To: James Morse, Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
On 31/01/2019 16:20, James Morse wrote:
> Hi Amit,
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> The keys can be switched either inside an assembly or such
>> functions which do not have pointer authentication checks, so a GCC
>> attribute is added to enable it.
>>
>> A function ptrauth_keys_store is added which is similar to existing
>> function ptrauth_keys_switch but saves the key values in memory.
>> This may be useful for save/restore scenarios when CPU changes
>> privilege levels, suspend/resume etc.
>
>
>> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
>> index 15d4951..98441ce 100644
>> --- a/arch/arm64/include/asm/pointer_auth.h
>> +++ b/arch/arm64/include/asm/pointer_auth.h
>> @@ -11,6 +11,13 @@
>>
>> #ifdef CONFIG_ARM64_PTR_AUTH
>> /*
>> + * Compile the function without pointer authentication instructions. This
>> + * allows pointer authentication to be enabled/disabled within the function
>> + * (but leaves the function unprotected by pointer authentication).
>> + */
>> +#define __no_ptrauth __attribute__((target("sign-return-address=none")))
>
> The documentation[0] for this says 'none' is the default. Will this only
> take-effect once the kernel supports pointer-auth for the host? (Is this just
> documentation until then?)
Yes, I don't think this should be in this series, since we're not
building the kernel with pointer auth yet.
>
> ('noptrauth' would fit with 'notrace' slightly better)
(But worse with e.g. __noreturn, __notrace_funcgraph, __init,
__always_inline, __exception. Not sure what the pattern is. Would
__noptrauth be better?)
Thanks,
Kristina
>
> [0]
> https://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html#AArch64-Function-Attributes
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-01-28 6:58 ` [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value Amit Daniel Kachhap
2019-01-31 16:22 ` James Morse
@ 2019-02-13 17:34 ` Kristina Martsenko
2019-02-14 11:03 ` Amit Daniel Kachhap
1 sibling, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:34 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
> is a constant value. This works today, as the host HCR_EL2 value is
> always the same, but this will get in the way of supporting extensions
> that require HCR_EL2 bits to be set conditionally for the host.
>
> To allow such features to work without KVM having to explicitly handle
> every possible host feature combination, this patch has KVM save/restore
> the host HCR when switching to/from a guest HCR. The saving of the
> register is done once during cpu hypervisor initialization state and is
> just restored after switch from guest.
Why is this patch needed? I couldn't find anything in this series that
sets HCR_EL2 conditionally for the host. It seems like the kernel still
always sets it to HCR_HOST_VHE_FLAGS/HCR_HOST_NVHE_FLAGS.
Looking back at v2 of the userspace pointer auth series, it seems that
the API/APK bits were set conditionally [1], so this patch would have
been needed to preserve HCR_EL2. But as of v3 of that series, the bits
have been set unconditionally through HCR_HOST_NVHE_FLAGS [2].
Is there something else I've missed?
Thanks,
Kristina
[1] https://lore.kernel.org/linux-arm-kernel/20171127163806.31435-6-mark.rutland@arm.com/
[2] https://lore.kernel.org/linux-arm-kernel/20180417183735.56985-5-mark.rutland@arm.com/
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
2019-01-28 14:25 ` Julien Thierry
2019-01-31 16:25 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register James Morse
@ 2019-02-13 17:34 ` Kristina Martsenko
2019-02-14 11:06 ` Amit Daniel Kachhap
2 siblings, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:34 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
Hi Amit,
(Please always Cc: everyone who commented on previous versions of the
series.)
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When pointer authentication is supported, a guest may wish to use it.
> This patch adds the necessary KVM infrastructure for this to work, with
> a semi-lazy context switch of the pointer auth state.
>
> Pointer authentication feature is only enabled when VHE is built
> into the kernel and present into CPU implementation so only VHE code
> paths are modified.
>
> When we schedule a vcpu, we disable guest usage of pointer
> authentication instructions and accesses to the keys. While these are
> disabled, we avoid context-switching the keys. When we trap the guest
> trying to use pointer authentication functionality, we change to eagerly
> context-switching the keys, and enable the feature. The next time the
> vcpu is scheduled out/in, we start again.
>
> Pointer authentication consists of address authentication and generic
> authentication, and CPUs in a system might have varied support for
> either. Where support for either feature is not uniform, it is hidden
> from guests via ID register emulation, as a result of the cpufeature
> framework in the host.
>
> Unfortunately, address authentication and generic authentication cannot
> be trapped separately, as the architecture provides a single EL2 trap
> covering both. If we wish to expose one without the other, we cannot
> prevent a (badly-written) guest from intermittently using a feature
> which is not uniformly supported (when scheduled on a physical CPU which
> supports the relevant feature). When the guest is scheduled on a
> physical CPU lacking the feature, these attempts will result in an UNDEF
> being taken by the guest.
[...]
> /*
> + * Handle the guest trying to use a ptrauth instruction, or trying to access a
> + * ptrauth register.
> + */
> +void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
> +{
> + if (has_vhe() && kvm_supports_ptrauth())
> + kvm_arm_vcpu_ptrauth_enable(vcpu);
> + else
> + kvm_inject_undefined(vcpu);
> +}
> +
> +/*
> * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
> - * a NOP).
> + * a NOP), or guest EL1 access to a ptrauth register.
Doesn't guest EL1 access of ptrauth registers go through trap_ptrauth
instead?
> */
> static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> - /*
> - * We don't currently support ptrauth in a guest, and we mask the ID
> - * registers to prevent well-behaved guests from trying to make use of
> - * it.
> - *
> - * Inject an UNDEF, as if the feature really isn't present.
> - */
> - kvm_inject_undefined(vcpu);
> + kvm_arm_vcpu_ptrauth_trap(vcpu);
> return 1;
> }
>
[...]
> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
> +{
> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
We don't call this code in the !VHE case anymore, so are the __hyp_text
annotations still needed?
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> + ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
[...]
> @@ -1040,14 +1066,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
> kvm_debug("SVE unsupported for guests, suppressing\n");
>
> val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
> - } else if (id == SYS_ID_AA64ISAR1_EL1) {
> - const u64 ptrauth_mask = (0xfUL << ID_AA64ISAR1_APA_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_API_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
> - (0xfUL << ID_AA64ISAR1_GPI_SHIFT);
> - if (val & ptrauth_mask)
> - kvm_debug("ptrauth unsupported for guests, suppressing\n");
> - val &= ~ptrauth_mask;
If all CPUs support address authentication, but no CPUs support generic
authentication, then I think the guest will still see address auth in
the ID register and try to use it, but since kvm_supports_ptrauth() ==
false, then kvm will enable trapping and inject an undef. So I think we
still need to zero the ID register bits here if !kvm_supports_ptrauth().
In the following patch, I think we also need to zero the bits if
!kvm_arm_vcpu_ptrauth_allowed(), as done in v4 [1], because otherwise
the guest will see that ptrauth is available, but will receive an undef
when it tries to use it.
Regarding the patch in v4, most of the work is passing the vcpu down to
read_id_reg(). Dave has a similar patch in his SVE series [2]. I think
it might make sense to rebase onto that patch and mention that patch as
a dependency in the cover letter.
[1] https://lore.kernel.org/linux-arm-kernel/1545119810-12182-5-git-send-email-amit.kachhap@arm.com/
[2] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-11-git-send-email-Dave.Martin@arm.com/
Thanks,
Kristina
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
2019-01-31 16:25 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register James Morse
@ 2019-02-13 17:35 ` Kristina Martsenko
2019-02-15 4:00 ` Amit Daniel Kachhap
2019-02-14 10:16 ` Amit Daniel Kachhap
1 sibling, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:35 UTC (permalink / raw)
To: James Morse, Amit Daniel Kachhap
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
On 31/01/2019 16:25, James Morse wrote:
> Hi Amit,
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> When pointer authentication is supported, a guest may wish to use it.
>> This patch adds the necessary KVM infrastructure for this to work, with
>> a semi-lazy context switch of the pointer auth state.
[...]
>> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
>> + struct kvm_cpu_context *host_ctxt,
>> + struct kvm_cpu_context *guest_ctxt)
>> +{
>> + if (!__ptrauth_is_enabled(vcpu))
>> + return;
>> +
>
>> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>
> We can't cast part of an array to a structure like this. What happens if the
> compiler inserts padding in struct-ptrauth_keys, or the struct randomization
> thing gets hold of it: https://lwn.net/Articles/722293/
>
> If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
> the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
> that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
> of the sys_reg array.
If I've understood correctly, the idea is to have a struct ptrauth_keys
in struct kvm_vcpu_arch, instead of having the keys in the
kvm_cpu_context->sys_regs array. This is to avoid having similar code in
__ptrauth_key_install/ptrauth_keys_switch and
__ptrauth_restore_key/__ptrauth_restore_state, and so that future
patches (that add pointer auth in the kernel) would only need to update
one place instead of two.
But it also means we'll have to special case pointer auth in
kvm_arm_sys_reg_set_reg/kvm_arm_sys_reg_get_reg and kvm_vcpu_arch. Is it
worth it? I'd prefer to keep the slight code duplication but avoid the
special casing.
>
>
> Wouldn't the host keys be available somewhere else? (they must get transfer to
> secondary CPUs somehow). Can we skip the save step when switching from the host?
>
>
>> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>> +}
>
[...]
>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 03b36f1..301d332 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>> sysreg_restore_guest_state_vhe(guest_ctxt);
>> __debug_switch_to_guest(vcpu);
>>
>> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
>> +
>> __set_guest_arch_workaround_state(vcpu);
>>
>> do {
>> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>>
>> __set_host_arch_workaround_state(vcpu);
>>
>> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
>> +
>> sysreg_save_guest_state_vhe(guest_ctxt);
>>
>> __deactivate_traps(vcpu);
>
> ...This makes me nervous...
>
> __guest_enter() is a function that (might) change the keys, then we change them
> again here. We can't have any signed return address between these two points. I
> don't trust the compiler not to generate any.
>
> ~
>
> I had a chat with some friendly compiler folk... because there are two identical
> sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
> move the common code to a function it then calls. Apparently this is called
> 'function outlining'.
>
> If the compiler does this, and the guest changes the keys, I think we would fail
> the return address check.
>
> Painting the whole thing with no_prauth would solve this, but this code then
> becomes a target.
> Because the compiler can't anticipate the keys changing, we ought to treat them
> the same way we do the callee saved registers, stack-pointer etc, and
> save/restore them in the __guest_enter() assembly code.
>
> (we can still keep the save/restore in C, but call it from assembly so we know
> nothing new is going on the stack).
I agree that this should be called from assembly if we were building the
kernel with pointer auth. But as we are not doing that yet in this
series, can't we keep the calls in kvm_vcpu_run_vhe for now?
In general I would prefer if the keys were switched in
kvm_arch_vcpu_load/put for now, since the keys are currently only used
in userspace. Once in-kernel pointer auth support comes along, it can
move the switch into kvm_vcpu_run_vhe or __guest_enter/__guest_exit as
required.
Thanks,
Kristina
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
2019-01-28 14:35 ` Julien Thierry
2019-01-31 16:27 ` James Morse
@ 2019-02-13 17:35 ` Kristina Martsenko
2019-02-15 4:49 ` Amit Daniel Kachhap
2 siblings, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:35 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> This feature will allow the KVM guest to allow the handling of
> pointer authentication instructions or to treat them as undefined
> if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
> supply this parameter instead of creating a new API.
>
> A new register is not created to pass this parameter via
> SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
> supplied is enough to enable this feature.
[...]
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index b200c14..b6950df 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -346,6 +346,10 @@ static inline int kvm_arm_have_ssbd(void)
> static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
> +static inline bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
It seems like this is only ever called from arm64 code, so do we need an
arch/arm/ definition?
> +/**
> + * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
> + *
> + * @vcpu: The VCPU pointer
> + *
> + * This function will be used to enable/disable ptrauth in guest as configured
> + * by the KVM userspace API.
> + */
> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
> +{
> + return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
> +}
I'm not sure, but should there also be something like
if (test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features) &&
!kvm_supports_ptrauth())
return -EINVAL;
in kvm_reset_vcpu?
Thanks,
Kristina
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers
2019-01-28 6:58 ` [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers Amit Daniel Kachhap
@ 2019-02-13 17:35 ` Kristina Martsenko
2019-02-13 17:54 ` Dave P Martin
0 siblings, 1 reply; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-13 17:35 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> According to userspace settings, ptrauth key registers are conditionally
> present in guest system register list based on user specified flag
> KVM_ARM_VCPU_PTRAUTH.
>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> Documentation/arm64/pointer-authentication.txt | 3 ++
> arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++-------
> 2 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
> index 0529a7d..3be4ee1 100644
> --- a/Documentation/arm64/pointer-authentication.txt
> +++ b/Documentation/arm64/pointer-authentication.txt
> @@ -87,3 +87,6 @@ created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
> to be enabled. Without this flag, pointer authentication is not enabled
> in KVM guests and attempted use of the feature will result in an UNDEFINED
> exception being injected into the guest.
> +
> +Additionally, when KVM_ARM_VCPU_PTRAUTH is not set then KVM will filter
> +out the authentication key registers from userspace.
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2546a65..b46a78e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1334,12 +1334,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
> { SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
>
> - PTRAUTH_KEY(APIA),
> - PTRAUTH_KEY(APIB),
> - PTRAUTH_KEY(APDA),
> - PTRAUTH_KEY(APDB),
> - PTRAUTH_KEY(APGA),
> -
> { SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
> { SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
> { SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
> @@ -1491,6 +1485,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x70 },
> };
>
> +static const struct sys_reg_desc ptrauth_reg_descs[] = {
> + PTRAUTH_KEY(APIA),
> + PTRAUTH_KEY(APIB),
> + PTRAUTH_KEY(APDA),
> + PTRAUTH_KEY(APDB),
> + PTRAUTH_KEY(APGA),
> +};
> +
> static bool trap_dbgidr(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> @@ -2093,6 +2095,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> r = find_reg(params, table, num);
> if (!r)
> r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> + if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> + r = find_reg(params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>
> if (likely(r)) {
> perform_access(vcpu, params, r);
> @@ -2206,6 +2210,8 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
> r = find_reg_by_id(id, ¶ms, table, num);
> if (!r)
> r = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> + if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> + r = find_reg(¶ms, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>
> /* Not saved in the sys_reg array and not otherwise accessible? */
> if (r && !(r->reg || r->get_user))
> @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
> }
>
> /* Assumed ordered tables, see kvm_sys_reg_table_init. */
> -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
> + const struct sys_reg_desc *desc, unsigned int len)
> {
> const struct sys_reg_desc *i1, *i2, *end1, *end2;
> unsigned int total = 0;
> size_t num;
> int err;
>
> + if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
> + return total;
> +
> /* We check for duplicates here, to allow arch-specific overrides. */
> i1 = get_target_table(vcpu->arch.target, true, &num);
> end1 = i1 + num;
> - i2 = sys_reg_descs;
> - end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
> + i2 = desc;
> + end2 = desc + len;
>
> BUG_ON(i1 == end1 || i2 == end2);
>
> @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
> {
> return ARRAY_SIZE(invariant_sys_regs)
> + num_demux_regs()
> - + walk_sys_regs(vcpu, (u64 __user *)NULL);
> + + walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
> + ARRAY_SIZE(sys_reg_descs))
> + + walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
> + ARRAY_SIZE(ptrauth_reg_descs));
> }
>
> int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> uindices++;
> }
>
> - err = walk_sys_regs(vcpu, uindices);
> + err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> + if (err < 0)
> + return err;
> + uindices += err;
> +
> + err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
> if (err < 0)
> return err;
> uindices += err;
> @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
> BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
> BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
> BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
> + BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
>
> /* We abuse the reset function to overwrite the table itself. */
> for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>
> /* Generic chip reset first (so target could override). */
> reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> + reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>
> table = get_target_table(vcpu->arch.target, true, &num);
> reset_sys_reg_descs(vcpu, table, num);
This isn't very scalable, since we'd need to duplicate all the above
code every time we add new system registers that are conditionally
accessible.
It seems that the SVE patches [1] solved this problem by adding a
check_present() callback into struct sys_reg_desc. It probably makes
sense to rebase onto that patch and just implement the callback for the
ptrauth key registers as well.
[1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/
Thanks,
Kristina
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers
2019-02-13 17:35 ` Kristina Martsenko
@ 2019-02-13 17:54 ` Dave P Martin
2019-02-15 4:57 ` Amit Daniel Kachhap
0 siblings, 1 reply; 31+ messages in thread
From: Dave P Martin @ 2019-02-13 17:54 UTC (permalink / raw)
To: Kristina Martsenko
Cc: Amit Kachhap, linux-arm-kernel, Christoffer Dall, Marc Zyngier,
Catalin Marinas, Will Deacon, Andrew Jones, Ramana Radhakrishnan,
kvmarm, linux-kernel, Mark Rutland
On Wed, Feb 13, 2019 at 05:35:46PM +0000, Kristina Martsenko wrote:
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> > According to userspace settings, ptrauth key registers are conditionally
> > present in guest system register list based on user specified flag
> > KVM_ARM_VCPU_PTRAUTH.
> >
> > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> > Cc: kvmarm@lists.cs.columbia.edu
> > Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> > Documentation/arm64/pointer-authentication.txt | 3 ++
> > arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++-------
> > 2 files changed, 34 insertions(+), 11 deletions(-)
> >
[...]
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
[...]
> > @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
> > }
> >
> > /* Assumed ordered tables, see kvm_sys_reg_table_init. */
> > -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> > +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
> > +const struct sys_reg_desc *desc, unsigned int len)
> > {
> > const struct sys_reg_desc *i1, *i2, *end1, *end2;
> > unsigned int total = 0;
> > size_t num;
> > int err;
> >
> > +if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
> > +return total;
> > +
> > /* We check for duplicates here, to allow arch-specific overrides. */
> > i1 = get_target_table(vcpu->arch.target, true, &num);
> > end1 = i1 + num;
> > -i2 = sys_reg_descs;
> > -end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
> > +i2 = desc;
> > +end2 = desc + len;
> >
> > BUG_ON(i1 == end1 || i2 == end2);
> >
> > @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
> > {
> > return ARRAY_SIZE(invariant_sys_regs)
> > + num_demux_regs()
> > -+ walk_sys_regs(vcpu, (u64 __user *)NULL);
> > ++ walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
> > +ARRAY_SIZE(sys_reg_descs))
> > ++ walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
> > +ARRAY_SIZE(ptrauth_reg_descs));
> > }
> >
> > int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > uindices++;
> > }
> >
> > -err = walk_sys_regs(vcpu, uindices);
> > +err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > +if (err < 0)
> > +return err;
> > +uindices += err;
> > +
> > +err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
> > if (err < 0)
> > return err;
> > uindices += err;
> > @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
> > BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
> > BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
> > BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
> > +BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
> >
> > /* We abuse the reset function to overwrite the table itself. */
> > for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> > @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> >
> > /* Generic chip reset first (so target could override). */
> > reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > +reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
> >
> > table = get_target_table(vcpu->arch.target, true, &num);
> > reset_sys_reg_descs(vcpu, table, num);
>
> This isn't very scalable, since we'd need to duplicate all the above
> code every time we add new system registers that are conditionally
> accessible.
Agreed, putting feature-specific checks in walk_sys_regs() is probably
best avoided. Over time we would likely accumulate a fair number of
these checks.
> It seems that the SVE patches [1] solved this problem by adding a
> check_present() callback into struct sys_reg_desc. It probably makes
> sense to rebase onto that patch and just implement the callback for the
> ptrauth key registers as well.
>
> [1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/
Note, I'm currently refactoring this so that enumeration through
KVM_GET_REG_LIST can be disabled independently of access to the
register. This may not be the best approach, but for SVE I have a
feature-specific ID register to handle too (ID_AA64ZFR0_EL1), which
needs to be hidden from the enumeration but still accessible with
read-as-zero behaviour.
This changes the API a bit: I move to a .restrictions() callback which
returns flags to say what is disabled, and this callback is used in the
common code so that you don't have repeat your "feature present" check
in every accessor, as is currently the case.
I'm aiming to post a respun series in the next day or two. The code
may of course change again after it gets reviewed...
Basing on [1] is probably a reasonable starting point, though.
Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-01-31 16:22 ` James Morse
@ 2019-02-14 9:49 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 9:49 UTC (permalink / raw)
To: James Morse
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi James,
Little late in replying as some issue in my mail settings.
On 1/31/19 9:52 PM, James Morse wrote:
> Hi Amit,
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
>> is a constant value. This works today, as the host HCR_EL2 value is
>> always the same, but this will get in the way of supporting extensions
>> that require HCR_EL2 bits to be set conditionally for the host.
>>
>> To allow such features to work without KVM having to explicitly handle
>> every possible host feature combination, this patch has KVM save/restore
>> the host HCR when switching to/from a guest HCR. The saving of the
>> register is done once during cpu hypervisor initialization state and is
>> just restored after switch from guest.
>>
>> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using
>> kvm_call_hyp and is helpful in NHVE case.
>
>> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated
>> to toggle the TGE bit with a RMW sequence, as we already do in
>> __tlb_switch_to_guest_vhe().
>
>
>> While at it, host MDCR_EL2 value is fetched in a similar way and restored
>> after every switch from host to guest. There should not be any change in
>> functionality due to this.
>
> Could this step be done as a separate subsequent patch? It would make review
> easier! The MDCR stuff would be a simplification if done second, done in one go
> like this its pretty noisy.
Ok, agree.
>
> There ought to be some justification for moving hcr/mdcr into the cpu_context in
> the commit message.
ohh I missed adding in commit. Just added in cover letter.
>
>
> If you're keeping Mark's 'Signed-off-by' its would be normal to keep Mark as the
> author in git. This shows up a an extra 'From:' when you post the patch, and
> gets picked up when the maintainer runs git-am.
>
> This patch has changed substantially from Mark's version:
> https://lkml.org/lkml/2017/11/27/675
>
> If you keep the signed-off-by, could you add a [note] in the signed-off area
> with a terse summary. Something like:
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> [ Move hcr to cpu_context, added __cpu_copy_hyp_conf()]
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>
> (9c06602b1b92 is a good picked-at-random example for both of these)
Thanks for the information.
>
>
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index f5b79e9..2da6e43 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void);
>>
>> extern u32 __kvm_get_mdcr_el2(void);
>>
>> +extern u64 __kvm_get_hcr_el2(void);
>
> Do we need these in separate helpers? For non-vhe this means two separate trips
> to EL2. Something like kvm_populate_host_context(void), and an __ version for
> the bit at EL2?
yes one wrapper for each of them will do.
>
> We don't need to pass the host-context to EL2 as once kvm is loaded we can
> access host per-cpu variables at EL2 using __hyp_this_cpu_read(). This will save
> passing the vcpu around.
>
>
>> @@ -458,6 +457,25 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>>
>> static inline void __cpu_init_stage2(void) {}
>>
>> +/**
>> + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers
>> + *
>> + * It is called once per-cpu during CPU hyp initialisation.
>> + */
>> +static inline void __cpu_copy_hyp_conf(void)
>> +{
>> + kvm_cpu_context_t *host_cxt = this_cpu_ptr(&kvm_host_cpu_state);
>> +
>> + host_cxt->hcr_el2 = kvm_call_hyp(__kvm_get_hcr_el2);
>> +
>> + /*
>> + * Retrieve the initial value of mdcr_el2 so we can preserve
>> + * MDCR_EL2.HPMN which has presumably been set-up by some
>> + * knowledgeable bootcode.
>> + */
>> + host_cxt->mdcr_el2 = kvm_call_hyp(__kvm_get_mdcr_el2);
>> +}
>
> Its strange to make this an inline in a header. kvm_arm_init_debug() is a
> static-inline for arch/arm, but a regular C function for arch/arm64. Can't we do
> the same?
>
>
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 68d6f7c..22c854a 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -316,3 +316,14 @@ void __hyp_text __kvm_enable_ssbs(void)
>> "msr sctlr_el2, %0"
>> : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS));
>> }
>> +
>> +/**
>> + * __read_hyp_hcr_el2 - Returns hcr_el2 register value
>> + *
>> + * This function acts as a function handler parameter for kvm_call_hyp and
>> + * may be called from EL1 exception level to fetch the register value.
>> + */
>> +u64 __hyp_text __kvm_get_hcr_el2(void)
>> +{
>> + return read_sysreg(hcr_el2);
>> +}
>
> While I'm all in favour of kernel-doc comments for functions, it may be
> over-kill in this case!
>
>
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 9e350fd3..2d65ada 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -1327,10 +1327,10 @@ static void cpu_hyp_reinit(void)
>> else
>> cpu_init_hyp_mode(NULL);
>>
>> - kvm_arm_init_debug();
>> -
>> if (vgic_present)
>> kvm_vgic_init_cpu_hardware();
>> +
>> + __cpu_copy_hyp_conf();
>> }
>
> Was there a reason to make this call later than it originally was?
> (kvm_vgic_init_cpu_hardware() doesn't use any of those values, so its fine, just
> curious!)
Yes. Can be moved before.
//Amit D
>
>
> Thanks,
>
> James
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
2019-01-31 16:25 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register James Morse
2019-02-13 17:35 ` Kristina Martsenko
@ 2019-02-14 10:16 ` Amit Daniel Kachhap
1 sibling, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 10:16 UTC (permalink / raw)
To: James Morse
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi James,
On 1/31/19 9:55 PM, James Morse wrote:
> Hi Amit,
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> When pointer authentication is supported, a guest may wish to use it.
>> This patch adds the necessary KVM infrastructure for this to work, with
>> a semi-lazy context switch of the pointer auth state.
>>
>> Pointer authentication feature is only enabled when VHE is built
>> into the kernel and present into CPU implementation so only VHE code
>
> ~s/into/in the/?
>
>> paths are modified.
>>
>> When we schedule a vcpu, we disable guest usage of pointer
>> authentication instructions and accesses to the keys. While these are
>> disabled, we avoid context-switching the keys. When we trap the guest
>> trying to use pointer authentication functionality, we change to eagerly
>> context-switching the keys, and enable the feature. The next time the
>> vcpu is scheduled out/in, we start again.
>
>> Pointer authentication consists of address authentication and generic
>> authentication, and CPUs in a system might have varied support for
>> either. Where support for either feature is not uniform, it is hidden
>> from guests via ID register emulation, as a result of the cpufeature
>> framework in the host.
>
>
>> Unfortunately, address authentication and generic authentication cannot
>> be trapped separately, as the architecture provides a single EL2 trap
>> covering both. If we wish to expose one without the other, we cannot
>> prevent a (badly-written) guest from intermittently using a feature
>> which is not uniformly supported (when scheduled on a physical CPU which
>> supports the relevant feature). When the guest is scheduled on a
>> physical CPU lacking the feature, these attempts will result in an UNDEF
>> being taken by the guest.
>
> This won't be fun. Can't KVM check that both are supported on all CPUs to avoid
> this? ...
The above message is confusing as both checks actually present. I will
update.
>
>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index dfcfba7..e1bf2a5 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
>> cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
>> }
>>
>> +static inline bool kvm_supports_ptrauth(void)
>> +{
>> + return system_supports_address_auth() && system_supports_generic_auth();
>> +}
>
> ... oh you do check. Could you cover this in the commit message? (to avoid an
> UNDEF being taken by the guest we ... )
>
> cpufeature.h is a strange place to put this, there are no other kvm symbols in
> there. But there are users of system_supports_foo() in kvm_host.h.
ok will check.
>
>
>> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
>> new file mode 100644
>> index 0000000..0576c01
>> --- /dev/null
>> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
>> @@ -0,0 +1,44 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
>> + *
>> + * Copyright 2018 Arm Limited
>> + * Author: Mark Rutland <mark.rutland@arm.com>
>> + * Amit Daniel Kachhap <amit.kachhap@arm.com>
>> + */
>> +#include <linux/compiler.h>
>> +#include <linux/kvm_host.h>
>> +
>> +#include <asm/cpucaps.h>
>> +#include <asm/cpufeature.h>
>> +#include <asm/kvm_asm.h>
>> +#include <asm/kvm_hyp.h>
>> +#include <asm/pointer_auth.h>
>> +
>> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
>
> Why __always_inline? Doesn't the compiler decide for 'static' symbols in C files?
This is to make the function pointer authentication safe. Although it
placed before key switch so may not be required.
>
>
>> +{
>> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
>> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
>> +}
>> +
>> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
>> + struct kvm_cpu_context *host_ctxt,
>> + struct kvm_cpu_context *guest_ctxt)
>> +{
>> + if (!__ptrauth_is_enabled(vcpu))
>> + return;
>> +
>
>> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>
> We can't cast part of an array to a structure like this. What happens if the
> compiler inserts padding in struct-ptrauth_keys, or the struct randomization
> thing gets hold of it: https://lwn.net/Articles/722293/
Yes this has got issue.
>
> If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
> the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
> that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
> of the sys_reg array.
ok.
>
>
> Wouldn't the host keys be available somewhere else? (they must get transfer to
> secondary CPUs somehow). Can we skip the save step when switching from the host?
Yes Host save can be done during vcpu_load and it works fine. However it
does not work during hypervisor configuration stage(i.e where HCR_EL2 is
saved/restored now) as keys are different.
>
>> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>> +}
>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 1f2d237..c798d0c 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -439,6 +451,18 @@ static inline bool kvm_arch_requires_vhe(void)
>> return false;
>> }
>>
>> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
>> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
>> +
>> +static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
>> +{
>> + /* Disable ptrauth and use it in a lazy context via traps */
>> + if (has_vhe() && kvm_supports_ptrauth())
>> + kvm_arm_vcpu_ptrauth_disable(vcpu);
>> +}
>
> (Could you move 'has_vhe()' into kvm_supports_ptrauth()? It fits with testing
> the other cpufeatures, and makes this a little more readable when you add
> 'allowed' to it later.)
yes.
>
>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 03b36f1..301d332 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>> sysreg_restore_guest_state_vhe(guest_ctxt);
>> __debug_switch_to_guest(vcpu);
>>
>> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
>> +
>> __set_guest_arch_workaround_state(vcpu);
>>
>> do {
>> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>>
>> __set_host_arch_workaround_state(vcpu);
>>
>> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
>> +
>> sysreg_save_guest_state_vhe(guest_ctxt);
>>
>> __deactivate_traps(vcpu);
>
> ...This makes me nervous...
>
> __guest_enter() is a function that (might) change the keys, then we change them
> again here. We can't have any signed return address between these two points. I
> don't trust the compiler not to generate any.
>
> ~
>
> I had a chat with some friendly compiler folk... because there are two identical
> sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
> move the common code to a function it then calls. Apparently this is called
> 'function outlining'.
>
> If the compiler does this, and the guest changes the keys, I think we would fail
> the return address check.
>
> Painting the whole thing with no_prauth would solve this, but this code then
> becomes a target.
> Because the compiler can't anticipate the keys changing, we ought to treat them
> the same way we do the callee saved registers, stack-pointer etc, and
> save/restore them in the __guest_enter() assembly code.
>
> (we can still keep the save/restore in C, but call it from assembly so we know
> nothing new is going on the stack).
I checked this and it seems easy to put inside guest_enter/guest_exit.
//Amit D
>
>
> Thanks,
>
> James
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-01-31 16:27 ` James Morse
@ 2019-02-14 10:47 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 10:47 UTC (permalink / raw)
To: James Morse
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
Kristina Martsenko, kvmarm, Ramana Radhakrishnan, Dave Martin,
linux-kernel
Hi,
On 1/31/19 9:57 PM, James Morse wrote:
> Hi Amit,
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> This feature will allow the KVM guest to allow the handling of
>> pointer authentication instructions or to treat them as undefined
>> if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
>> supply this parameter instead of creating a new API.
>>
>> A new register is not created to pass this parameter via
>> SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
>> supplied is enough to enable this feature.
>
>
>> diff --git a/Documentation/arm64/pointer-authentication.txt
> b/Documentation/arm64/pointer-authentication.txt
>> index a25cd21..0529a7d 100644
>> --- a/Documentation/arm64/pointer-authentication.txt
>> +++ b/Documentation/arm64/pointer-authentication.txt
>> @@ -82,7 +82,8 @@ pointers).
>> Virtualization
>> --------------
>>
>> -Pointer authentication is not currently supported in KVM guests. KVM
>> -will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of
>> -the feature will result in an UNDEFINED exception being injected into
>> -the guest.
>> +Pointer authentication is enabled in KVM guest when virtual machine is
>> +created by passing a flag (KVM_ARM_VCPU_PTRAUTH)
>
> Isn't that a VCPU flag? Shouldn't this be when each VCPU is created?
Yes it is a VCPU flag.
>
>
>> requesting this feature
>> +to be enabled. Without this flag, pointer authentication is not enabled
>> +in KVM guests and attempted use of the feature will result in an UNDEFINED
>> +exception being injected into the guest.
>
> ... what happens if KVM's user-space enables ptrauth on some vcpus, but not on
> others?
Yes seems to be issue. Let me check more on this if there are other ways
of passing the userspace parameter such as in CREATE_VM type ioctl.
>
> You removed the id-register suppression in the previous patch, but it doesn't
> get hooked up to kvm_arm_vcpu_ptrauth_allowed() here. (you could add
> kvm_arm_vcpu_ptrauth_allowed() earlier, and default it to true to make it easier).
>
> Doesn't this mean that if the CPU supports pointer auth, but user-space doesn't
> specify this flag, the guest gets mysterious undef's whenever it tries to use
> the advertised feature?
Agree, ID registers should be masked when userspace disables it.
>
> (whether we support big/little virtual-machines is probably a separate issue,
> but the id registers need to be consistent with our trap-and-undef behaviour)
>
>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index c798d0c..4a6ec40 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -453,14 +453,15 @@ static inline bool kvm_arch_requires_vhe(void)
>>
>> void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
>> void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
>> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu);
>>
>> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
>> {
>> /* Disable ptrauth and use it in a lazy context via traps */
>> - if (has_vhe() && kvm_supports_ptrauth())
>> + if (has_vhe() && kvm_supports_ptrauth()
>> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
>> kvm_arm_vcpu_ptrauth_disable(vcpu);
>> }
>> -
>> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
>>
>
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 5b980e7..c0e5dcd 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -179,7 +179,8 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> */
>> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
>> {
>> - if (has_vhe() && kvm_supports_ptrauth())
>> + if (has_vhe() && kvm_supports_ptrauth()
>> + && kvm_arm_vcpu_ptrauth_allowed(vcpu))
>
> Duplication. If has_vhe() moved into kvm_supports_ptrauth(), and
> kvm_supports_ptrauth() was called from kvm_arm_vcpu_ptrauth_allowed() it would
> be clearer that use of this feature was becoming user-controlled policy.
>
> (We don't need to list the dependencies at every call site)
ok.
>
>
>> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
>> index 0576c01..369624f 100644
>> --- a/arch/arm64/kvm/hyp/ptrauth-sr.c
>> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
>> @@ -42,3 +42,16 @@ void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
>> ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>> ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>> }
>> +
>> +/**
>> + * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
>
> ('enabled by KVM's user-space' may be clearer. 'Present in vcpu' could be down
> to a cpufeature thing)
ok.
>
>
>> + *
>> + * @vcpu: The VCPU pointer
>> + *
>> + * This function will be used to enable/disable ptrauth in guest as configured
>
> ... but it just tests the bit ...
>
>> + * by the KVM userspace API.
>> + */
>> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
>> +{
>> + return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
>> +}
>
>
> Thanks,
>
> James
>
//Amit
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys
2019-02-13 17:32 ` Kristina Martsenko
@ 2019-02-14 10:53 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 10:53 UTC (permalink / raw)
To: Kristina Martsenko, James Morse
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
Hi,
On 2/13/19 11:02 PM, Kristina Martsenko wrote:
> On 31/01/2019 16:20, James Morse wrote:
>> Hi Amit,
>>
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> The keys can be switched either inside an assembly or such
>>> functions which do not have pointer authentication checks, so a GCC
>>> attribute is added to enable it.
>>>
>>> A function ptrauth_keys_store is added which is similar to existing
>>> function ptrauth_keys_switch but saves the key values in memory.
>>> This may be useful for save/restore scenarios when CPU changes
>>> privilege levels, suspend/resume etc.
>>
>>
>>> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
>>> index 15d4951..98441ce 100644
>>> --- a/arch/arm64/include/asm/pointer_auth.h
>>> +++ b/arch/arm64/include/asm/pointer_auth.h
>>> @@ -11,6 +11,13 @@
>>>
>>> #ifdef CONFIG_ARM64_PTR_AUTH
>>> /*
>>> + * Compile the function without pointer authentication instructions. This
>>> + * allows pointer authentication to be enabled/disabled within the function
>>> + * (but leaves the function unprotected by pointer authentication).
>>> + */
>>> +#define __no_ptrauth __attribute__((target("sign-return-address=none")))
>>
>> The documentation[0] for this says 'none' is the default. Will this only
>> take-effect once the kernel supports pointer-auth for the host? (Is this just
>> documentation until then?)
>
> Yes, I don't think this should be in this series, since we're not
> building the kernel with pointer auth yet.
I added it to stress on some functions which should be pointer
authentication safe. Yes this can be dropped and a small comment may
also do.
//Amit D
>
>>
>> ('noptrauth' would fit with 'notrace' slightly better)
>
> (But worse with e.g. __noreturn, __notrace_funcgraph, __init,
> __always_inline, __exception. Not sure what the pattern is. Would
> __noptrauth be better?)
>
> Thanks,
> Kristina
>
>>
>> [0]
>> https://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html#AArch64-Function-Attributes
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-02-13 17:34 ` Kristina Martsenko
@ 2019-02-14 11:03 ` Amit Daniel Kachhap
2019-02-15 15:50 ` Kristina Martsenko
0 siblings, 1 reply; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 11:03 UTC (permalink / raw)
To: Kristina Martsenko, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
Hi,
On 2/13/19 11:04 PM, Kristina Martsenko wrote:
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
>> is a constant value. This works today, as the host HCR_EL2 value is
>> always the same, but this will get in the way of supporting extensions
>> that require HCR_EL2 bits to be set conditionally for the host.
>>
>> To allow such features to work without KVM having to explicitly handle
>> every possible host feature combination, this patch has KVM save/restore
>> the host HCR when switching to/from a guest HCR. The saving of the
>> register is done once during cpu hypervisor initialization state and is
>> just restored after switch from guest.
>
> Why is this patch needed? I couldn't find anything in this series that
> sets HCR_EL2 conditionally for the host. It seems like the kernel still
> always sets it to HCR_HOST_VHE_FLAGS/HCR_HOST_NVHE_FLAGS.
This patch is not directly related to pointer authentication but just a
helper to optimize save/restore. In this way save may be avoided for
each switch and only restore is done. Patch 3 does sets HCR_EL2 in VHE_RUN.
>
> Looking back at v2 of the userspace pointer auth series, it seems that
> the API/APK bits were set conditionally [1], so this patch would have
> been needed to preserve HCR_EL2. But as of v3 of that series, the bits
> have been set unconditionally through HCR_HOST_NVHE_FLAGS [2].
>
> Is there something else I've missed?
Now HCR_EL2 is modified during switch time and NHVE doesnt support
ptrauth so [2] doesn't makes sense.
//Amit D
>
> Thanks,
> Kristina
>
> [1] https://lore.kernel.org/linux-arm-kernel/20171127163806.31435-6-mark.rutland@arm.com/
> [2] https://lore.kernel.org/linux-arm-kernel/20180417183735.56985-5-mark.rutland@arm.com/
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers
2019-02-13 17:34 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Kristina Martsenko
@ 2019-02-14 11:06 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-14 11:06 UTC (permalink / raw)
To: Kristina Martsenko, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
Hi,
On 2/13/19 11:04 PM, Kristina Martsenko wrote:
> Hi Amit,
>
> (Please always Cc: everyone who commented on previous versions of the
> series.)
>
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> When pointer authentication is supported, a guest may wish to use it.
>> This patch adds the necessary KVM infrastructure for this to work, with
>> a semi-lazy context switch of the pointer auth state.
>>
>> Pointer authentication feature is only enabled when VHE is built
>> into the kernel and present into CPU implementation so only VHE code
>> paths are modified.
>>
>> When we schedule a vcpu, we disable guest usage of pointer
>> authentication instructions and accesses to the keys. While these are
>> disabled, we avoid context-switching the keys. When we trap the guest
>> trying to use pointer authentication functionality, we change to eagerly
>> context-switching the keys, and enable the feature. The next time the
>> vcpu is scheduled out/in, we start again.
>>
>> Pointer authentication consists of address authentication and generic
>> authentication, and CPUs in a system might have varied support for
>> either. Where support for either feature is not uniform, it is hidden
>> from guests via ID register emulation, as a result of the cpufeature
>> framework in the host.
>>
>> Unfortunately, address authentication and generic authentication cannot
>> be trapped separately, as the architecture provides a single EL2 trap
>> covering both. If we wish to expose one without the other, we cannot
>> prevent a (badly-written) guest from intermittently using a feature
>> which is not uniformly supported (when scheduled on a physical CPU which
>> supports the relevant feature). When the guest is scheduled on a
>> physical CPU lacking the feature, these attempts will result in an UNDEF
>> being taken by the guest.
>
> [...]
>
>> /*
>> + * Handle the guest trying to use a ptrauth instruction, or trying to access a
>> + * ptrauth register.
>> + */
>> +void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
>> +{
>> + if (has_vhe() && kvm_supports_ptrauth())
>> + kvm_arm_vcpu_ptrauth_enable(vcpu);
>> + else
>> + kvm_inject_undefined(vcpu);
>> +}
>> +
>> +/*
>> * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
>> - * a NOP).
>> + * a NOP), or guest EL1 access to a ptrauth register.
>
> Doesn't guest EL1 access of ptrauth registers go through trap_ptrauth
> instead?
Yes you are right.
>
>> */
>> static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> {
>> - /*
>> - * We don't currently support ptrauth in a guest, and we mask the ID
>> - * registers to prevent well-behaved guests from trying to make use of
>> - * it.
>> - *
>> - * Inject an UNDEF, as if the feature really isn't present.
>> - */
>> - kvm_inject_undefined(vcpu);
>> + kvm_arm_vcpu_ptrauth_trap(vcpu);
>> return 1;
>> }
>>
>
> [...]
>
>> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
>> +{
>> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
>> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
>> +}
>> +
>> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
>> + struct kvm_cpu_context *host_ctxt,
>> + struct kvm_cpu_context *guest_ctxt)
>> +{
>> + if (!__ptrauth_is_enabled(vcpu))
>> + return;
>> +
>> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>> +}
>> +
>> +void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
>
> We don't call this code in the !VHE case anymore, so are the __hyp_text
> annotations still needed?
Yes they can be removed.
>
>> + struct kvm_cpu_context *host_ctxt,
>> + struct kvm_cpu_context *guest_ctxt)
>> +{
>> + if (!__ptrauth_is_enabled(vcpu))
>> + return;
>> +
>> + ptrauth_keys_store((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>> + ptrauth_keys_switch((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>> +}
>
> [...]
>
>> @@ -1040,14 +1066,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
>> kvm_debug("SVE unsupported for guests, suppressing\n");
>>
>> val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>> - } else if (id == SYS_ID_AA64ISAR1_EL1) {
>> - const u64 ptrauth_mask = (0xfUL << ID_AA64ISAR1_APA_SHIFT) |
>> - (0xfUL << ID_AA64ISAR1_API_SHIFT) |
>> - (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
>> - (0xfUL << ID_AA64ISAR1_GPI_SHIFT);
>> - if (val & ptrauth_mask)
>> - kvm_debug("ptrauth unsupported for guests, suppressing\n");
>> - val &= ~ptrauth_mask;
>
> If all CPUs support address authentication, but no CPUs support generic
> authentication, then I think the guest will still see address auth in
> the ID register and try to use it, but since kvm_supports_ptrauth() ==
> false, then kvm will enable trapping and inject an undef. So I think we
> still need to zero the ID register bits here if !kvm_supports_ptrauth().
Yes even James Morse suggested same thing.
>
> In the following patch, I think we also need to zero the bits if
> !kvm_arm_vcpu_ptrauth_allowed(), as done in v4 [1], because otherwise
> the guest will see that ptrauth is available, but will receive an undef
> when it tries to use it.
ok.
>
> Regarding the patch in v4, most of the work is passing the vcpu down to
> read_id_reg(). Dave has a similar patch in his SVE series [2]. I think
> it might make sense to rebase onto that patch and mention that patch as
> a dependency in the cover letter.
Yes it is helpful. Will check it.
//Amit D
>
> [1] https://lore.kernel.org/linux-arm-kernel/1545119810-12182-5-git-send-email-amit.kachhap@arm.com/
> [2] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-11-git-send-email-Dave.Martin@arm.com/
>
> Thanks,
> Kristina
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
2019-02-13 17:35 ` Kristina Martsenko
@ 2019-02-15 4:00 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-15 4:00 UTC (permalink / raw)
To: Kristina Martsenko, James Morse
Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
kvmarm, Ramana Radhakrishnan, Dave Martin, linux-kernel
Hi,
On 2/13/19 11:05 PM, Kristina Martsenko wrote:
> On 31/01/2019 16:25, James Morse wrote:
>> Hi Amit,
>>
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> When pointer authentication is supported, a guest may wish to use it.
>>> This patch adds the necessary KVM infrastructure for this to work, with
>>> a semi-lazy context switch of the pointer auth state.
>
> [...]
>
>>> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
>>> + struct kvm_cpu_context *host_ctxt,
>>> + struct kvm_cpu_context *guest_ctxt)
>>> +{
>>> + if (!__ptrauth_is_enabled(vcpu))
>>> + return;
>>> +
>>
>>> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
>>
>> We can't cast part of an array to a structure like this. What happens if the
>> compiler inserts padding in struct-ptrauth_keys, or the struct randomization
>> thing gets hold of it: https://lwn.net/Articles/722293/
>>
>> If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
>> the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
>> that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
>> of the sys_reg array.
>
> If I've understood correctly, the idea is to have a struct ptrauth_keys
> in struct kvm_vcpu_arch, instead of having the keys in the
> kvm_cpu_context->sys_regs array. This is to avoid having similar code in
> __ptrauth_key_install/ptrauth_keys_switch and
> __ptrauth_restore_key/__ptrauth_restore_state, and so that future
> patches (that add pointer auth in the kernel) would only need to update
> one place instead of two.
Yes your observation is correct.
>
> But it also means we'll have to special case pointer auth in
> kvm_arm_sys_reg_set_reg/kvm_arm_sys_reg_get_reg and kvm_vcpu_arch. Is it
> worth it? I'd prefer to keep the slight code duplication but avoid the
> special casing.
In my local implementation I implemented above by separating ptrauth
registers from sys registers but if I use the new way suggested by Dave
[1] then those things are not possible as reg ID is used for matching
entries.
So I will stick to the single sys_reg list for next iteration using [1].
[1]:
https://lore.kernel.org/linux-arm-kernel/1547757219-19439-11-git-send-email-Dave.Martin@arm.com/
>
>>
>>
>> Wouldn't the host keys be available somewhere else? (they must get transfer to
>> secondary CPUs somehow). Can we skip the save step when switching from the host?
>>
>>
>>> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
>>> +}
>>
>
> [...]
>
>>
>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>> index 03b36f1..301d332 100644
>>> --- a/arch/arm64/kvm/hyp/switch.c
>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>>> sysreg_restore_guest_state_vhe(guest_ctxt);
>>> __debug_switch_to_guest(vcpu);
>>>
>>> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
>>> +
>>> __set_guest_arch_workaround_state(vcpu);
>>>
>>> do {
>>> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>>>
>>> __set_host_arch_workaround_state(vcpu);
>>>
>>> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
>>> +
>>> sysreg_save_guest_state_vhe(guest_ctxt);
>>>
>>> __deactivate_traps(vcpu);
>>
>> ...This makes me nervous...
>>
>> __guest_enter() is a function that (might) change the keys, then we change them
>> again here. We can't have any signed return address between these two points. I
>> don't trust the compiler not to generate any.
>>
>> ~
>>
>> I had a chat with some friendly compiler folk... because there are two identical
>> sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
>> move the common code to a function it then calls. Apparently this is called
>> 'function outlining'.
>>
>> If the compiler does this, and the guest changes the keys, I think we would fail
>> the return address check.
>>
>> Painting the whole thing with no_prauth would solve this, but this code then
>> becomes a target.
>> Because the compiler can't anticipate the keys changing, we ought to treat them
>> the same way we do the callee saved registers, stack-pointer etc, and
>> save/restore them in the __guest_enter() assembly code.
>>
>> (we can still keep the save/restore in C, but call it from assembly so we know
>> nothing new is going on the stack).
>
> I agree that this should be called from assembly if we were building the
> kernel with pointer auth. But as we are not doing that yet in this
> series, can't we keep the calls in kvm_vcpu_run_vhe for now?
Well if we keep them in kvm_vcpu_run_vhe then there is not much issue
also in calling those C functions from assembly guest_enter/guest_exit.
It works fine in my local implementation. This will also save code
churning again when kernel ptrauth support is added. The only extra
change required to be done is to assign attribute _noptrauth to those
functions. I will add these comments properly in function description.
>
> In general I would prefer if the keys were switched in
> kvm_arch_vcpu_load/put for now, since the keys are currently only used
> in userspace. Once in-kernel pointer auth support comes along, it can
> move the switch into kvm_vcpu_run_vhe or __guest_enter/__guest_exit as
> required.
Yes it is possible but then there are other benefits in doing this way.
It will be always ptrauth registers save/restore if inside
kvm_arch_vcpu_load/put even if userspace does not use it. The current
way does save/restore dynamically when userspace uses ptrauth
instructions by trapping first time.
Some discussion happened on it earlier between Mark and Cristopher and
they seem to agree on it [2].
[2]: https://lore.kernel.org/lkml/20180409125818.GE10904@cbox/
Thanks,
Amit D
>
> Thanks,
> Kristina
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
2019-02-13 17:35 ` Kristina Martsenko
@ 2019-02-15 4:49 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-15 4:49 UTC (permalink / raw)
To: Kristina Martsenko, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
Hi,
On 2/13/19 11:05 PM, Kristina Martsenko wrote:
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>> This feature will allow the KVM guest to allow the handling of
>> pointer authentication instructions or to treat them as undefined
>> if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
>> supply this parameter instead of creating a new API.
>>
>> A new register is not created to pass this parameter via
>> SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
>> supplied is enough to enable this feature.
>
> [...]
>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index b200c14..b6950df 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -346,6 +346,10 @@ static inline int kvm_arm_have_ssbd(void)
>> static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
>> static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
>> static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
>> +static inline bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
>> +{
>> + return false;
>> +}
>
> It seems like this is only ever called from arm64 code, so do we need an
> arch/arm/ definition?
Yes not required. Nice catch.
>
>> +/**
>> + * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
>> + *
>> + * @vcpu: The VCPU pointer
>> + *
>> + * This function will be used to enable/disable ptrauth in guest as configured
>> + * by the KVM userspace API.
>> + */
>> +bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
>> +{
>> + return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
>> +}
>
> I'm not sure, but should there also be something like
>
> if (test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features) &&
> !kvm_supports_ptrauth())
> return -EINVAL;
>
> in kvm_reset_vcpu?
Yes makes sense. I missed it and with Dave martin patch this may be done
cleanly.
Thanks,
Amit D
>
> Thanks,
> Kristina
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers
2019-02-13 17:54 ` Dave P Martin
@ 2019-02-15 4:57 ` Amit Daniel Kachhap
0 siblings, 0 replies; 31+ messages in thread
From: Amit Daniel Kachhap @ 2019-02-15 4:57 UTC (permalink / raw)
To: Dave P Martin, Kristina Martsenko
Cc: linux-arm-kernel, Christoffer Dall, Marc Zyngier,
Catalin Marinas, Will Deacon, Andrew Jones, Ramana Radhakrishnan,
kvmarm, linux-kernel, Mark Rutland
Hi,
On 2/13/19 11:24 PM, Dave P Martin wrote:
> On Wed, Feb 13, 2019 at 05:35:46PM +0000, Kristina Martsenko wrote:
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> According to userspace settings, ptrauth key registers are conditionally
>>> present in guest system register list based on user specified flag
>>> KVM_ARM_VCPU_PTRAUTH.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
>>> Cc: kvmarm@lists.cs.columbia.edu
>>> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> ---
>>> Documentation/arm64/pointer-authentication.txt | 3 ++
>>> arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++-------
>>> 2 files changed, 34 insertions(+), 11 deletions(-)
>>>
>
> [...]
>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>
> [...]
>
>>> @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
>>> }
>>>
>>> /* Assumed ordered tables, see kvm_sys_reg_table_init. */
>>> -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
>>> +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
>>> + const struct sys_reg_desc *desc, unsigned int len)
>>> {
>>> const struct sys_reg_desc *i1, *i2, *end1, *end2;
>>> unsigned int total = 0;
>>> size_t num;
>>> int err;
>>>
>>> + if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
>>> + return total;
>>> +
>>> /* We check for duplicates here, to allow arch-specific overrides. */
>>> i1 = get_target_table(vcpu->arch.target, true, &num);
>>> end1 = i1 + num;
>>> - i2 = sys_reg_descs;
>>> - end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
>>> + i2 = desc;
>>> + end2 = desc + len;
>>>
>>> BUG_ON(i1 == end1 || i2 == end2);
>>>
>>> @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
>>> {
>>> return ARRAY_SIZE(invariant_sys_regs)
>>> + num_demux_regs()
>>> - + walk_sys_regs(vcpu, (u64 __user *)NULL);
>>> + + walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
>>> + ARRAY_SIZE(sys_reg_descs))
>>> + + walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
>>> + ARRAY_SIZE(ptrauth_reg_descs));
>>> }
>>>
>>> int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> uindices++;
>>> }
>>>
>>> - err = walk_sys_regs(vcpu, uindices);
>>> + err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> + if (err < 0)
>>> + return err;
>>> + uindices += err;
>>> +
>>> + err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>> if (err < 0)
>>> return err;
>>> uindices += err;
>>> @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
>>> BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
>>> BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
>>> BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
>>> + BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
>>>
>>> /* We abuse the reset function to overwrite the table itself. */
>>> for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
>>> @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>>
>>> /* Generic chip reset first (so target could override). */
>>> reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> + reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>>
>>> table = get_target_table(vcpu->arch.target, true, &num);
>>> reset_sys_reg_descs(vcpu, table, num);
>>
>> This isn't very scalable, since we'd need to duplicate all the above
>> code every time we add new system registers that are conditionally
>> accessible.
>
> Agreed, putting feature-specific checks in walk_sys_regs() is probably
> best avoided. Over time we would likely accumulate a fair number of
> these checks.
>
>> It seems that the SVE patches [1] solved this problem by adding a
>> check_present() callback into struct sys_reg_desc. It probably makes
>> sense to rebase onto that patch and just implement the callback for the
>> ptrauth key registers as well.
>>
>> [1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/
>
> Note, I'm currently refactoring this so that enumeration through
> KVM_GET_REG_LIST can be disabled independently of access to the
> register. This may not be the best approach, but for SVE I have a
> feature-specific ID register to handle too (ID_AA64ZFR0_EL1), which
> needs to be hidden from the enumeration but still accessible with
> read-as-zero behaviour.
>
> This changes the API a bit: I move to a .restrictions() callback which
> returns flags to say what is disabled, and this callback is used in the
> common code so that you don't have repeat your "feature present" check
> in every accessor, as is currently the case.
>
> I'm aiming to post a respun series in the next day or two. The code
> may of course change again after it gets reviewed...
>
>
> Basing on [1] is probably a reasonable starting point, though.
Thanks Kristina and Dave for this pointer. I will rebase my next
iteration based on it.
Thanks,
Amit
>
> Cheers
> ---Dave
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
2019-02-14 11:03 ` Amit Daniel Kachhap
@ 2019-02-15 15:50 ` Kristina Martsenko
0 siblings, 0 replies; 31+ messages in thread
From: Kristina Martsenko @ 2019-02-15 15:50 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-arm-kernel
Cc: Christoffer Dall, Marc Zyngier, Catalin Marinas, Will Deacon,
Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
linux-kernel, Mark Rutland
On 14/02/2019 11:03, Amit Daniel Kachhap wrote:
> Hi,
>
> On 2/13/19 11:04 PM, Kristina Martsenko wrote:
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
>>> is a constant value. This works today, as the host HCR_EL2 value is
>>> always the same, but this will get in the way of supporting extensions
>>> that require HCR_EL2 bits to be set conditionally for the host.
>>>
>>> To allow such features to work without KVM having to explicitly handle
>>> every possible host feature combination, this patch has KVM save/restore
>>> the host HCR when switching to/from a guest HCR. The saving of the
>>> register is done once during cpu hypervisor initialization state and is
>>> just restored after switch from guest.
>>
>> Why is this patch needed? I couldn't find anything in this series that
>> sets HCR_EL2 conditionally for the host. It seems like the kernel still
>> always sets it to HCR_HOST_VHE_FLAGS/HCR_HOST_NVHE_FLAGS.
>
> This patch is not directly related to pointer authentication but just a
> helper to optimize save/restore. In this way save may be avoided for
> each switch and only restore is done. Patch 3 does sets HCR_EL2 in VHE_RUN.
Patch 3 sets the HCR_EL2.{API,APK} bits for the *guest*, not the host.
This patch here adds saving/restoring for the *host* HCR_EL2. As far as
I can tell, the value of the host HCR_EL2 never changes.
Regarding save/restore, currently the kernel never saves the host
HCR_EL2, because it always restores HCR_EL2 to HCR_HOST_{,N}VHE_FLAGS (a
constant value!) when returning to the host. With this patch, we
effectively just save HCR_HOST_{,N}VHE_FLAGS into kvm_host_cpu_state,
and restore it from there when returning to the host.
Unless we actually change the host HCR_EL2 value to something other than
HCR_HOST_{,N}VHE_FLAGS somewhere in this series, this patch is unnecessary.
>>
>> Looking back at v2 of the userspace pointer auth series, it seems that
>> the API/APK bits were set conditionally [1], so this patch would have
>> been needed to preserve HCR_EL2. But as of v3 of that series, the bits
>> have been set unconditionally through HCR_HOST_NVHE_FLAGS [2].
>>
>> Is there something else I've missed?
> Now HCR_EL2 is modified during switch time and NHVE doesnt support
> ptrauth so [2] doesn't makes sense.
In case of NVHE, we do support pointer auth in the *host* userspace, so
the patch [2] is necessary. In case of NVHE we do not support pointer
auth for KVM *guests*.
Thanks,
Kristina
>> [1] https://lore.kernel.org/linux-arm-kernel/20171127163806.31435-6-mark.rutland@arm.com/
>> [2] https://lore.kernel.org/linux-arm-kernel/20180417183735.56985-5-mark.rutland@arm.com/
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2019-02-15 15:50 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-28 6:58 [PATCH v5 0/6] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys Amit Daniel Kachhap
2019-01-31 16:20 ` James Morse
2019-02-13 17:32 ` Kristina Martsenko
2019-02-14 10:53 ` Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value Amit Daniel Kachhap
2019-01-31 16:22 ` James Morse
2019-02-14 9:49 ` Amit Daniel Kachhap
2019-02-13 17:34 ` Kristina Martsenko
2019-02-14 11:03 ` Amit Daniel Kachhap
2019-02-15 15:50 ` Kristina Martsenko
2019-01-28 6:58 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Amit Daniel Kachhap
2019-01-28 14:25 ` Julien Thierry
2019-01-31 16:25 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register James Morse
2019-02-13 17:35 ` Kristina Martsenko
2019-02-15 4:00 ` Amit Daniel Kachhap
2019-02-14 10:16 ` Amit Daniel Kachhap
2019-02-13 17:34 ` [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers Kristina Martsenko
2019-02-14 11:06 ` Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication Amit Daniel Kachhap
2019-01-28 14:35 ` Julien Thierry
2019-01-31 16:27 ` James Morse
2019-02-14 10:47 ` Amit Daniel Kachhap
2019-02-13 17:35 ` Kristina Martsenko
2019-02-15 4:49 ` Amit Daniel Kachhap
2019-01-28 6:58 ` [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers Amit Daniel Kachhap
2019-02-13 17:35 ` Kristina Martsenko
2019-02-13 17:54 ` Dave P Martin
2019-02-15 4:57 ` Amit Daniel Kachhap
2019-01-28 6:58 ` [kvmtool PATCH v5 6/6] arm/kvm: arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
2019-01-28 14:56 ` Julien Thierry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).