linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest
@ 2019-04-23  4:42 Amit Daniel Kachhap
  2019-04-23  4:42 ` [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest Amit Daniel Kachhap
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

Hi,

This patch series adds pointer authentication support for KVM guest and
is based on top of Linux kvmarm/next repo. 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.

The current v10 patch series contains review comments and suggestions by
Dave Martin and Mark Zyngier.

Major changes since v9 [11]:

* Move ptrauth enable, disable and setup_lazy function from C file to H file with
  static inline attribute.
* Add cpufeature static checks at many places to optimize for non-ptrauth systems.

Changes since v8 [10]: Major changes are listed below and detail changes are in
		      each patch.
* Added a new vcpu specific arch flag to control enabling/disabling ptrauth.
* Patches restructured as 3 patches related to hcr_el2, mdcr_el2 and
  hyp_symbol_addr cleanup and optimization dropped. They will be posted separately.

Changes since v7 [9]: Major changes are listed below and detail changes are in
		      each patch.
 * Comments and Documentation updated to reflect using address/generic
   features flag together.
 * Dropped the documentation patch and added those details in the relevant
   patches.
 * Rebased the patch series on 2 patches of Dave Martin v6 SVE series. 
 * Small bug fixes.

Changes since v6 [8]: Major changes are listed below.

* Pointer authentication key switch entirely in assembly now.
* isb instruction added after Key switched to host.
* Use __hyp_this_cpu_ptr for both VHE and nVHE mode.
* 2 separate flags for address and generic authentication.
* kvm_arm_vcpu_ptrauth_allowed renamed to has_vcpu_ptrauth.
* kvm_arm_vcpu_ptrauth_reset renamed to kvm_arm_vcpu_ptrauth_setup_lazy.
* Save of host Key registers now done in ptrauth instruction trap.
* A fix to add kern_hyp_va to get correct host_ctxt pointer in nVHE mode.
* Patches re-structured to better reflect ABI change.

Changes since v5 [7]: Major changes are listed below.

* Split hcr_el2 and mdcr_el2 save/restore in two patches.
* Reverted back save/restore of sys-reg keys as done in V4 [5]. There was
  suggestion by James Morse to use ptrauth utilities in a single place
  in arm core and use them from kvm. However this change deviates from the
  existing sys-reg implementations and is not scalable.
* Invoked the key switch C functions from __guest_enter/__guest_exit assembly.
* Host key save is now done inside vcpu_load.
* Reverted back masking of cpufeature ID registers for ptrauth when disabled
  from userpace.
* Reset of ptrauth key registers not done conditionally.
* Code and Documentation cleanup.

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
[7]: https://lkml.org/lkml/2019/1/28/49
[8]: https://lkml.org/lkml/2019/2/19/190 
[9]: https://lkml.org/lkml/2019/3/19/125 
[10]: https://lkml.org/lkml/2019/4/1/1595
[11]: https://lkml.org/lkml/2019/4/11/1072 

Linux (5.1-rc2 based kvmarm/next repo):

Amit Daniel Kachhap (3):
  KVM: arm64: Add a vcpu flag to control ptrauth for guest
  KVM: arm64: Add userspace flag to enable pointer authentication
  KVM: arm64: Add capability to advertise ptrauth for guest

Mark Rutland (1):
  KVM: arm/arm64: context-switch ptrauth registers

 Documentation/arm64/pointer-authentication.txt |  22 ++++-
 Documentation/virtual/kvm/api.txt              |  16 ++++
 arch/arm/include/asm/kvm_emulate.h             |   2 +
 arch/arm64/Kconfig                             |   5 +-
 arch/arm64/include/asm/kvm_emulate.h           |  16 ++++
 arch/arm64/include/asm/kvm_host.h              |  21 ++++-
 arch/arm64/include/asm/kvm_ptrauth.h           | 108 +++++++++++++++++++++++++
 arch/arm64/include/uapi/asm/kvm.h              |   2 +
 arch/arm64/kernel/asm-offsets.c                |   6 ++
 arch/arm64/kvm/handle_exit.c                   |  36 +++++++--
 arch/arm64/kvm/hyp/entry.S                     |  15 ++++
 arch/arm64/kvm/reset.c                         |  32 ++++++++
 arch/arm64/kvm/sys_regs.c                      |  43 +++++++++-
 include/uapi/linux/kvm.h                       |   2 +
 virt/kvm/arm/arm.c                             |   2 +
 15 files changed, 310 insertions(+), 18 deletions(-)
 create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h

kvmtool:

Repo: git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
Amit Daniel Kachhap (1):
  KVM: arm/arm64: Add a vcpu feature for pointer authentication

 arm/aarch32/include/kvm/kvm-cpu-arch.h    |  1 +
 arm/aarch64/include/asm/kvm.h             |  2 ++
 arm/aarch64/include/kvm/kvm-config-arch.h |  6 +++++-
 arm/aarch64/include/kvm/kvm-cpu-arch.h    |  2 ++
 arm/include/arm-common/kvm-config-arch.h  |  2 ++
 arm/kvm-cpu.c                             | 20 ++++++++++++++++++--
 include/linux/kvm.h                       |  2 ++
 7 files changed, 32 insertions(+), 3 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest
  2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
@ 2019-04-23  4:42 ` Amit Daniel Kachhap
  2019-04-23 15:44   ` Dave Martin
  2019-04-23  4:42 ` [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers Amit Daniel Kachhap
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

A per vcpu flag is added to check if pointer authentication is
enabled for the vcpu or not. This flag may be enabled according to
the necessary user policies and host capabilities.

This patch also adds a helper to check the flag.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
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: kvmarm@lists.cs.columbia.edu
---
Changes since v9:

* Added ptrauth cpufeature static check in vcpu_has_ptrauth [Marc Zyngier].

 arch/arm64/include/asm/kvm_host.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7a096fd..7ccac42 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -355,10 +355,15 @@ struct kvm_vcpu_arch {
 #define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
 #define KVM_ARM64_GUEST_HAS_SVE		(1 << 5) /* SVE exposed to guest */
 #define KVM_ARM64_VCPU_SVE_FINALIZED	(1 << 6) /* SVE config completed */
+#define KVM_ARM64_GUEST_HAS_PTRAUTH	(1 << 7) /* PTRAUTH exposed to guest */
 
 #define vcpu_has_sve(vcpu) (system_supports_sve() && \
 			    ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_SVE))
 
+#define vcpu_has_ptrauth(vcpu)	((system_supports_address_auth() || \
+				  system_supports_generic_auth()) && \
+				 ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_PTRAUTH))
+
 #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
 
 /*
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
  2019-04-23  4:42 ` [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest Amit Daniel Kachhap
@ 2019-04-23  4:42 ` Amit Daniel Kachhap
  2019-04-23  9:39   ` Marc Zyngier
  2019-04-24 13:39   ` Dave Martin
  2019-04-23  4:42 ` [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication Amit Daniel Kachhap
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

From: Mark Rutland <mark.rutland@arm.com>

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
in the kernel and present in the 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. However the host key save is
optimized and implemented inside ptrauth instruction/register access
trap.

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). Hence, this patch expects both type of
authentication to be present in a cpu.

This switch of key is done from guest enter/exit assembly as preparation
for the upcoming in-kernel pointer authentication support. Hence, these
key switching routines are not implemented in C code as they may cause
pointer authentication key signing error in some situations.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
[Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
, save host key in ptrauth exception trap]
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
---
Changes since v9:

* Removed hardcoding of enum values[Mark Zyngier].
* Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
* Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
* Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
  kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
* Added/Modified comments at few places [Marc Zyngier].

 arch/arm/include/asm/kvm_emulate.h   |   2 +
 arch/arm64/Kconfig                   |   5 +-
 arch/arm64/include/asm/kvm_emulate.h |  16 ++++++
 arch/arm64/include/asm/kvm_host.h    |  14 +++++
 arch/arm64/include/asm/kvm_ptrauth.h | 108 +++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/asm-offsets.c      |   6 ++
 arch/arm64/kvm/handle_exit.c         |  36 +++++++++---
 arch/arm64/kvm/hyp/entry.S           |  15 +++++
 arch/arm64/kvm/sys_regs.c            |  43 +++++++++++++-
 virt/kvm/arm/arm.c                   |   2 +
 10 files changed, 234 insertions(+), 13 deletions(-)
 create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 8927cae..efb0e2c 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -343,4 +343,6 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 	}
 }
 
+static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu) {}
+
 #endif /* __ARM_KVM_EMULATE_H__ */
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9e..3cfe2eb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
 	  context-switched along with the process.
 
 	  The feature is detected at runtime. If the feature is not present in
-	  hardware it will not be advertised to userspace nor will it be
-	  enabled.
+	  hardware it will not be advertised to userspace/KVM guest nor will it
+	  be enabled. However, KVM guest also require VHE mode and hence
+	  CONFIG_ARM64_VHE=y option to use this feature.
 
 endmenu
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d384279..613427f 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -98,6 +98,22 @@ static inline void vcpu_set_wfe_traps(struct kvm_vcpu *vcpu)
 	vcpu->arch.hcr_el2 |= HCR_TWE;
 }
 
+static inline void vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr_el2 |= (HCR_API | HCR_APK);
+}
+
+static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
+}
+
+static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu)
+{
+	if (vcpu_has_ptrauth(vcpu))
+		vcpu_ptrauth_disable(vcpu);
+}
+
 static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.vsesr_el2;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7ccac42..7eebea7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -161,6 +161,18 @@ enum vcpu_sysreg {
 	PMSWINC_EL0,	/* Software Increment Register */
 	PMUSERENR_EL0,	/* User Enable Register */
 
+	/* Pointer Authentication Registers in a strict increasing order. */
+	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 */
@@ -530,6 +542,8 @@ static inline bool kvm_arch_requires_vhe(void)
 	return false;
 }
 
+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_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h
new file mode 100644
index 0000000..f337237
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_ptrauth.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* arch/arm64/include/asm/kvm_ptrauth.h: Guest/host ptrauth save/restore
+ * Copyright 2019 Arm Limited
+ * Authors: Mark Rutland <mark.rutland@arm.com>
+ *         Amit Daniel Kachhap <amit.kachhap@arm.com>
+ */
+
+#ifndef __ASM_KVM_PTRAUTH_H
+#define __ASM_KVM_PTRAUTH_H
+
+#ifdef __ASSEMBLY__
+
+#include <asm/sysreg.h>
+
+#ifdef	CONFIG_ARM64_PTR_AUTH
+
+#define PTRAUTH_REG_OFFSET(x)	(x - CPU_APIAKEYLO_EL1)
+
+/*
+ * CPU_AP*_EL1 values exceed immediate offset range (512) for stp instruction
+ * so below macros takes CPU_APIAKEYLO_EL1 as base and calculates the offset of
+ * the keys from this base to avoid an extra add instruction. These macros
+ * assumes the keys offsets are aligned in a specific increasing order.
+ */
+.macro	ptrauth_save_state base, reg1, reg2
+	mrs_s	\reg1, SYS_APIAKEYLO_EL1
+	mrs_s	\reg2, SYS_APIAKEYHI_EL1
+	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
+	mrs_s	\reg1, SYS_APIBKEYLO_EL1
+	mrs_s	\reg2, SYS_APIBKEYHI_EL1
+	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
+	mrs_s	\reg1, SYS_APDAKEYLO_EL1
+	mrs_s	\reg2, SYS_APDAKEYHI_EL1
+	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
+	mrs_s	\reg1, SYS_APDBKEYLO_EL1
+	mrs_s	\reg2, SYS_APDBKEYHI_EL1
+	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
+	mrs_s	\reg1, SYS_APGAKEYLO_EL1
+	mrs_s	\reg2, SYS_APGAKEYHI_EL1
+	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
+.endm
+
+.macro	ptrauth_restore_state base, reg1, reg2
+	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
+	msr_s	SYS_APIAKEYLO_EL1, \reg1
+	msr_s	SYS_APIAKEYHI_EL1, \reg2
+	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
+	msr_s	SYS_APIBKEYLO_EL1, \reg1
+	msr_s	SYS_APIBKEYHI_EL1, \reg2
+	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
+	msr_s	SYS_APDAKEYLO_EL1, \reg1
+	msr_s	SYS_APDAKEYHI_EL1, \reg2
+	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
+	msr_s	SYS_APDBKEYLO_EL1, \reg1
+	msr_s	SYS_APDBKEYHI_EL1, \reg2
+	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
+	msr_s	SYS_APGAKEYLO_EL1, \reg1
+	msr_s	SYS_APGAKEYHI_EL1, \reg2
+.endm
+
+/* Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will check for
+ * the presence of one of the cpufeature flag ARM64_HAS_ADDRESS_AUTH_ARCH or
+ * ARM64_HAS_ADDRESS_AUTH_IMP_DEF and then proceed ahead with the save/restore
+ * of Pointer Authentication key registers.
+ */
+.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
+alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
+	b	1000f
+alternative_else_nop_endif
+alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
+	b	1001f
+alternative_else_nop_endif
+1000:
+	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
+	and	\reg1, \reg1, #(HCR_API | HCR_APK)
+	cbz	\reg1, 1001f
+	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
+	ptrauth_restore_state	\reg1, \reg2, \reg3
+1001:
+.endm
+
+.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
+alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
+	b	2000f
+alternative_else_nop_endif
+alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
+	b	2001f
+alternative_else_nop_endif
+2000:
+	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
+	and	\reg1, \reg1, #(HCR_API | HCR_APK)
+	cbz	\reg1, 2001f
+	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
+	ptrauth_save_state	\reg1, \reg2, \reg3
+	add	\reg1, \h_ctxt, #CPU_APIAKEYLO_EL1
+	ptrauth_restore_state	\reg1, \reg2, \reg3
+	isb
+2001:
+.endm
+
+#else /* !CONFIG_ARM64_PTR_AUTH */
+.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
+.endm
+.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
+.endm
+#endif /* CONFIG_ARM64_PTR_AUTH */
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_KVM_PTRAUTH_H */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 7f40dcb..8178330 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -125,7 +125,13 @@ int main(void)
   DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
   DEFINE(VCPU_FAULT_DISR,	offsetof(struct kvm_vcpu, arch.fault.disr_el1));
   DEFINE(VCPU_WORKAROUND_FLAGS,	offsetof(struct kvm_vcpu, arch.workaround_flags));
+  DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(CPU_GP_REGS,		offsetof(struct kvm_cpu_context, gp_regs));
+  DEFINE(CPU_APIAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIAKEYLO_EL1]));
+  DEFINE(CPU_APIBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIBKEYLO_EL1]));
+  DEFINE(CPU_APDAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDAKEYLO_EL1]));
+  DEFINE(CPU_APDBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDBKEYLO_EL1]));
+  DEFINE(CPU_APGAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APGAKEYLO_EL1]));
   DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_regs, regs));
   DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
 #endif
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 0b79834..516aead 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -173,20 +173,40 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+#define __ptrauth_save_key(regs, key)						\
+({										\
+	regs[key ## KEYLO_EL1] = read_sysreg_s(SYS_ ## key ## KEYLO_EL1);	\
+	regs[key ## KEYHI_EL1] = read_sysreg_s(SYS_ ## key ## KEYHI_EL1);	\
+})
+
+/*
+ * 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)
+{
+	struct kvm_cpu_context *ctxt;
+
+	if (vcpu_has_ptrauth(vcpu)) {
+		vcpu_ptrauth_enable(vcpu);
+		ctxt = vcpu->arch.host_cpu_context;
+		__ptrauth_save_key(ctxt->sys_regs, APIA);
+		__ptrauth_save_key(ctxt->sys_regs, APIB);
+		__ptrauth_save_key(ctxt->sys_regs, APDA);
+		__ptrauth_save_key(ctxt->sys_regs, APDB);
+		__ptrauth_save_key(ctxt->sys_regs, APGA);
+	} else {
+		kvm_inject_undefined(vcpu);
+	}
+}
+
 /*
  * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
  * a NOP).
  */
 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/entry.S b/arch/arm64/kvm/hyp/entry.S
index 675fdc1..93ba3d7 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -24,6 +24,7 @@
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/kvm_ptrauth.h>
 
 #define CPU_GP_REG_OFFSET(x)	(CPU_GP_REGS + x)
 #define CPU_XREG_OFFSET(x)	CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
@@ -64,6 +65,13 @@ ENTRY(__guest_enter)
 
 	add	x18, x0, #VCPU_CONTEXT
 
+	// Macro ptrauth_switch_to_guest format:
+	// 	ptrauth_switch_to_guest(guest cxt, tmp1, tmp2, tmp3)
+	// The below macro to restore guest keys is not implemented in C code
+	// as it may cause Pointer Authentication key signing mismatch errors
+	// when this feature is enabled for kernel code.
+	ptrauth_switch_to_guest x18, x0, x1, x2
+
 	// Restore guest regs x0-x17
 	ldp	x0, x1,   [x18, #CPU_XREG_OFFSET(0)]
 	ldp	x2, x3,   [x18, #CPU_XREG_OFFSET(2)]
@@ -118,6 +126,13 @@ ENTRY(__guest_exit)
 
 	get_host_ctxt	x2, x3
 
+	// Macro ptrauth_switch_to_guest format:
+	// 	ptrauth_switch_to_host(guest cxt, host cxt, tmp1, tmp2, tmp3)
+	// The below macro to save/restore keys is not implemented in C code
+	// as it may cause Pointer Authentication key signing mismatch errors
+	// when this feature is enabled for kernel code.
+	ptrauth_switch_to_host x1, x2, x3, x4, x5
+
 	// Now restore the host regs
 	restore_callee_saved_regs x2
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7046c76..7f06c2e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1007,6 +1007,35 @@ 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), }
 
+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 is false for both cases and PC remains same,
+	 * a) Re-execute the same key register access instruction after enabling
+	 *    ptrauth.
+	 * b) UNDEF is injected as ptrauth is not supported/enabled.
+	 */
+	return false;
+}
+
+static unsigned int ptrauth_visibility(const struct kvm_vcpu *vcpu,
+			const struct sys_reg_desc *rd)
+{
+	return vcpu_has_ptrauth(vcpu) ? 0 : REG_HIDDEN_USER | REG_HIDDEN_GUEST;
+}
+
+#define __PTRAUTH_KEY(k)						\
+	{ SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k,		\
+	.visibility = ptrauth_visibility}
+
+#define PTRAUTH_KEY(k)							\
+	__PTRAUTH_KEY(k ## KEYLO_EL1),					\
+	__PTRAUTH_KEY(k ## KEYHI_EL1)
+
 static bool access_arch_timer(struct kvm_vcpu *vcpu,
 			      struct sys_reg_params *p,
 			      const struct sys_reg_desc *r)
@@ -1058,9 +1087,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 					 (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 (!vcpu_has_ptrauth(vcpu)) {
+			if (val & ptrauth_mask)
+				kvm_debug("ptrauth unsupported for guests, suppressing\n");
+			val &= ~ptrauth_mask;
+		}
 	}
 
 	return val;
@@ -1460,6 +1491,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 7039c99c..156c09d 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -385,6 +385,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		vcpu_clear_wfe_traps(vcpu);
 	else
 		vcpu_set_wfe_traps(vcpu);
+
+	vcpu_ptrauth_setup_lazy(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication
  2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
  2019-04-23  4:42 ` [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest Amit Daniel Kachhap
  2019-04-23  4:42 ` [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers Amit Daniel Kachhap
@ 2019-04-23  4:42 ` Amit Daniel Kachhap
  2019-04-23 15:45   ` Dave Martin
  2019-04-23  4:42 ` [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest Amit Daniel Kachhap
  2019-04-23  4:42 ` [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
  4 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

Now that the building blocks of pointer authentication are present, lets
add userspace flags KVM_ARM_VCPU_PTRAUTH_ADDRESS and
KVM_ARM_VCPU_PTRAUTH_GENERIC. These flags will enable pointer
authentication for the KVM guest on a per-vcpu basis through the ioctl
KVM_ARM_VCPU_INIT.

This features will allow the KVM guest to allow the handling of
pointer authentication instructions or to treat them as undefined
if not set.

Necessary documentations are added to reflect the changes done.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
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: kvmarm@lists.cs.columbia.edu
---
Changed since v9:
* Fixed tab alignment at few places [Dave Martin].
* Split the system capability checks [Dave Martin].

 Documentation/arm64/pointer-authentication.txt | 22 +++++++++++++++++----
 Documentation/virtual/kvm/api.txt              | 10 ++++++++++
 arch/arm64/include/asm/kvm_host.h              |  2 +-
 arch/arm64/include/uapi/asm/kvm.h              |  2 ++
 arch/arm64/kvm/reset.c                         | 27 ++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
index 5baca42..fc71b33 100644
--- a/Documentation/arm64/pointer-authentication.txt
+++ b/Documentation/arm64/pointer-authentication.txt
@@ -87,7 +87,21 @@ used to get and set the keys for a thread.
 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 each virtual cpu is
+initialised by passing flags KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] and
+requesting these two separate cpu features to be enabled. The current KVM
+guest implementation works by enabling both features together, so both
+these userspace flags are checked before enabling pointer authentication.
+The separate userspace flag will allow to have no userspace ABI changes
+if support is added in the future to allow these two features to be
+enabled independently of one another.
+
+As Arm Architecture specifies that Pointer Authentication feature is
+implemented along with the VHE feature so KVM arm64 ptrauth code relies
+on VHE mode to be present.
+
+Additionally, when these vcpu feature flags are not set then KVM will
+filter out the Pointer Authentication system key registers from
+KVM_GET/SET_REG_* ioctls and mask those features from cpufeature ID
+register. Any attempt to use the Pointer Authentication instructions 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 e410a9f..32afe7f 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2761,6 +2761,16 @@ Possible features:
 	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
 	  Depends on KVM_CAP_ARM_PMU_V3.
 
+	- KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
+	  for arm64 only.
+	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
+	  must be requested or neither must be requested.
+
+	- KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
+	  for arm64 only.
+	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
+	  must be requested or neither must be requested.
+
 	- KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only).
 	  Depends on KVM_CAP_ARM_SVE.
 	  Requires KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE):
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7eebea7..f772ac2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -49,7 +49,7 @@
 
 #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
 
-#define KVM_VCPU_MAX_FEATURES 5
+#define KVM_VCPU_MAX_FEATURES 7
 
 #define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index edd2db8..7b7ac0f 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -104,6 +104,8 @@ 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 */
 #define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
+#define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
+#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
 
 struct kvm_vcpu_init {
 	__u32 target;
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 3402543..028d0c6 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -221,6 +221,27 @@ static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
 		memset(vcpu->arch.sve_state, 0, vcpu_sve_state_size(vcpu));
 }
 
+static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
+{
+	/* Support ptrauth only if the system supports these capabilities. */
+	if (!has_vhe())
+		return -EINVAL;
+
+	if (!system_supports_address_auth() ||
+	    !system_supports_generic_auth())
+		return -EINVAL;
+	/*
+	 * For now make sure that both address/generic pointer authentication
+	 * features are requested by the userspace together.
+	 */
+	if (!test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) ||
+	    !test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features))
+		return -EINVAL;
+
+	vcpu->arch.flags |= KVM_ARM64_GUEST_HAS_PTRAUTH;
+	return 0;
+}
+
 /**
  * kvm_reset_vcpu - sets core registers and sys_regs to reset value
  * @vcpu: The VCPU pointer
@@ -261,6 +282,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 		kvm_vcpu_reset_sve(vcpu);
 	}
 
+	if (test_bit(KVM_ARM_VCPU_PTRAUTH_ADDRESS, vcpu->arch.features) ||
+	    test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, vcpu->arch.features)) {
+		if (kvm_vcpu_enable_ptrauth(vcpu))
+			goto out;
+	}
+
 	switch (vcpu->arch.target) {
 	default:
 		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest
  2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
                   ` (2 preceding siblings ...)
  2019-04-23  4:42 ` [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication Amit Daniel Kachhap
@ 2019-04-23  4:42 ` Amit Daniel Kachhap
  2019-04-23 15:45   ` Dave Martin
  2019-04-23  4:42 ` [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
  4 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

This patch advertises the capability of two cpu feature called address
pointer authentication and generic pointer authentication. These
capabilities depend upon system support for pointer authentication and
VHE mode.

The current arm64 KVM partially implements pointer authentication and
support of address/generic authentication are tied together. However,
separate ABI requirements for both of them is added so that any future
isolated implementation will not require any ABI changes.

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: kvmarm@lists.cs.columbia.edu
---
Changes since v9:
* Fixed tab alignment issues [Dave Martin].
* Clarify the api documentation [Dave Martin].

 Documentation/virtual/kvm/api.txt | 14 ++++++++++----
 arch/arm64/kvm/reset.c            |  5 +++++
 include/uapi/linux/kvm.h          |  2 ++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 32afe7f..fac1887 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2763,13 +2763,19 @@ Possible features:
 
 	- KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
 	  for arm64 only.
-	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
-	  must be requested or neither must be requested.
+	  Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
+	  If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
+	  both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
+	  KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
+	  requested.
 
 	- KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
 	  for arm64 only.
-	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
-	  must be requested or neither must be requested.
+	  Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
+	  If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
+	  both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
+	  KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
+	  requested.
 
 	- KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only).
 	  Depends on KVM_CAP_ARM_SVE.
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 028d0c6..f0faf54 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -101,6 +101,11 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_SVE:
 		r = system_supports_sve();
 		break;
+	case KVM_CAP_ARM_PTRAUTH_ADDRESS:
+	case KVM_CAP_ARM_PTRAUTH_GENERIC:
+		r = has_vhe() && system_supports_address_auth() &&
+				 system_supports_generic_auth();
+		break;
 	default:
 		r = 0;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 1d56444..4dc34f8 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -989,6 +989,8 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
 #define KVM_CAP_HYPERV_CPUID 167
 #define KVM_CAP_ARM_SVE 168
+#define KVM_CAP_ARM_PTRAUTH_ADDRESS 169
+#define KVM_CAP_ARM_PTRAUTH_GENERIC 170
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
                   ` (3 preceding siblings ...)
  2019-04-23  4:42 ` [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest Amit Daniel Kachhap
@ 2019-04-23  4:42 ` Amit Daniel Kachhap
  2019-04-23 15:46   ` Dave Martin
  4 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23  4:42 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, James Morse, Julien Thierry

This patch adds a runtime capabality for KVM tool to enable Arm64 8.3
Pointer Authentication in guest kernel. Two vcpu features
KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable
Pointer Authentication in KVM guest after checking the capability.

Command line options --enable-ptrauth and --disable-ptrauth are added
to use this feature. However, if those options are not provided then
also this feature is enabled if host supports this capability.

The macros defined in the headers are not in sync and should be replaced
from the upstream.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---

Changes since v9:
* Added a error check for both enable-ptrauth and disable-ptrauth
  option.
* Make the error explicit when enable-ptrauth is provided [Dave Martin].

 arm/aarch32/include/kvm/kvm-cpu-arch.h    |  1 +
 arm/aarch64/include/asm/kvm.h             |  2 ++
 arm/aarch64/include/kvm/kvm-config-arch.h |  6 +++++-
 arm/aarch64/include/kvm/kvm-cpu-arch.h    |  2 ++
 arm/include/arm-common/kvm-config-arch.h  |  2 ++
 arm/kvm-cpu.c                             | 20 ++++++++++++++++++--
 include/linux/kvm.h                       |  2 ++
 7 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
index d28ea67..520ea76 100644
--- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
+++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
@@ -13,4 +13,5 @@
 #define ARM_CPU_ID		0, 0, 0
 #define ARM_CPU_ID_MPIDR	5
 
+#define ARM_VCPU_PTRAUTH_FEATURE	0
 #endif /* KVM__KVM_CPU_ARCH_H */
diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
index 97c3478..a2546e6 100644
--- a/arm/aarch64/include/asm/kvm.h
+++ b/arm/aarch64/include/asm/kvm.h
@@ -102,6 +102,8 @@ 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_ADDRESS	5 /* CPU uses address pointer authentication */
+#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* CPU uses generic pointer authentication */
 
 struct kvm_vcpu_init {
 	__u32 target;
diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
index 04be43d..0279b13 100644
--- a/arm/aarch64/include/kvm/kvm-config-arch.h
+++ b/arm/aarch64/include/kvm/kvm-config-arch.h
@@ -8,7 +8,11 @@
 			"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', "enable-ptrauth", &(cfg)->enable_ptrauth,	\
+			"Enables pointer authentication"),		\
+	OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth,	\
+			"Disables pointer 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..fcc2107 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
 
+#define ARM_VCPU_PTRAUTH_FEATURE	((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \
+					| (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC))
 #endif /* KVM__KVM_CPU_ARCH_H */
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index 5734c46..1b4287d 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -10,6 +10,8 @@ struct kvm_config_arch {
 	bool		aarch32_guest;
 	bool		has_pmuv3;
 	u64		kaslr_seed;
+	bool		enable_ptrauth;
+	bool		disable_ptrauth;
 	enum irqchip_type irqchip;
 	u64		fw_addr;
 };
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index 7780251..acd1d5f 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -68,6 +68,18 @@ 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);
 	}
 
+	/* Check Pointer Authentication command line arguments. */
+	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
+		die("Both enable-ptrauth and disable-ptrauth option cannot be present");
+	/*
+	 * Always enable Pointer Authentication if system supports
+	 * this extension unless disable-ptrauth option is present.
+	 */
+	if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+	    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) &&
+	    !kvm->cfg.arch.disable_ptrauth)
+			vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
+
 	/*
 	 * If the preferred target ioctl is successful then
 	 * use preferred target else try each and every target type
@@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 			die("Unable to find matching target");
 	}
 
-	if (err || target->init(vcpu))
-		die("Unable to initialise vcpu");
+	if (err || target->init(vcpu)) {
+		if (kvm->cfg.arch.enable_ptrauth)
+			die("Unable to initialise vcpu with pointer authentication feature");
+		else
+			die("Unable to initialise vcpu");
+	}
 
 	coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION,
 				 KVM_CAP_COALESCED_MMIO);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 6d4ea4b..de1033b 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -988,6 +988,8 @@ 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_ADDRESS 169
+#define KVM_CAP_ARM_PTRAUTH_GENERIC 170
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23  4:42 ` [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers Amit Daniel Kachhap
@ 2019-04-23  9:39   ` Marc Zyngier
  2019-04-23 10:24     ` Amit Daniel Kachhap
  2019-04-24 13:39   ` Dave Martin
  1 sibling, 1 reply; 26+ messages in thread
From: Marc Zyngier @ 2019-04-23  9:39 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Christoffer Dall, Catalin Marinas, Will Deacon,
	Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
	Kristina Martsenko, linux-kernel, Mark Rutland, James Morse,
	Julien Thierry

On Tue, 23 Apr 2019 05:42:35 +0100,
Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
> 
> From: Mark Rutland <mark.rutland@arm.com>
> 
> 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
> in the kernel and present in the 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. However the host key save is
> optimized and implemented inside ptrauth instruction/register access
> trap.
> 
> 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). Hence, this patch expects both type of
> authentication to be present in a cpu.
> 
> This switch of key is done from guest enter/exit assembly as preparation
> for the upcoming in-kernel pointer authentication support. Hence, these
> key switching routines are not implemented in C code as they may cause
> pointer authentication key signing error in some situations.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
> , save host key in ptrauth exception trap]
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> ---
> Changes since v9:
> 
> * Removed hardcoding of enum values[Mark Zyngier].
> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
> * Added/Modified comments at few places [Marc Zyngier].
> 
>  arch/arm/include/asm/kvm_emulate.h   |   2 +
>  arch/arm64/Kconfig                   |   5 +-
>  arch/arm64/include/asm/kvm_emulate.h |  16 ++++++
>  arch/arm64/include/asm/kvm_host.h    |  14 +++++
>  arch/arm64/include/asm/kvm_ptrauth.h | 108 +++++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/asm-offsets.c      |   6 ++
>  arch/arm64/kvm/handle_exit.c         |  36 +++++++++---
>  arch/arm64/kvm/hyp/entry.S           |  15 +++++
>  arch/arm64/kvm/sys_regs.c            |  43 +++++++++++++-
>  virt/kvm/arm/arm.c                   |   2 +
>  10 files changed, 234 insertions(+), 13 deletions(-)
>  create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 8927cae..efb0e2c 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -343,4 +343,6 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu) {}
> +
>  #endif /* __ARM_KVM_EMULATE_H__ */
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9e..3cfe2eb 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
>  	  context-switched along with the process.
>  
>  	  The feature is detected at runtime. If the feature is not present in
> -	  hardware it will not be advertised to userspace nor will it be
> -	  enabled.
> +	  hardware it will not be advertised to userspace/KVM guest nor will it
> +	  be enabled. However, KVM guest also require VHE mode and hence
> +	  CONFIG_ARM64_VHE=y option to use this feature.

SVE seems to have the exact same requirements, and has

	depends on !KVM || ARM64_VHE

Why don't we have that for PTR_AUTH too?

>  
>  endmenu
>  
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d384279..613427f 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -98,6 +98,22 @@ static inline void vcpu_set_wfe_traps(struct kvm_vcpu *vcpu)
>  	vcpu->arch.hcr_el2 |= HCR_TWE;
>  }
>  
> +static inline void vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcr_el2 |= (HCR_API | HCR_APK);
> +}
> +
> +static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
> +}
> +
> +static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu)
> +{
> +	if (vcpu_has_ptrauth(vcpu))
> +		vcpu_ptrauth_disable(vcpu);
> +}
> +
>  static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->arch.vsesr_el2;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7ccac42..7eebea7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -161,6 +161,18 @@ enum vcpu_sysreg {
>  	PMSWINC_EL0,	/* Software Increment Register */
>  	PMUSERENR_EL0,	/* User Enable Register */
>  
> +	/* Pointer Authentication Registers in a strict increasing order. */
> +	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 */
> @@ -530,6 +542,8 @@ static inline bool kvm_arch_requires_vhe(void)
>  	return false;
>  }
>  
> +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_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
> diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h
> new file mode 100644
> index 0000000..f337237
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_ptrauth.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* arch/arm64/include/asm/kvm_ptrauth.h: Guest/host ptrauth save/restore
> + * Copyright 2019 Arm Limited
> + * Authors: Mark Rutland <mark.rutland@arm.com>
> + *         Amit Daniel Kachhap <amit.kachhap@arm.com>
> + */
> +
> +#ifndef __ASM_KVM_PTRAUTH_H
> +#define __ASM_KVM_PTRAUTH_H
> +
> +#ifdef __ASSEMBLY__
> +
> +#include <asm/sysreg.h>
> +
> +#ifdef	CONFIG_ARM64_PTR_AUTH
> +
> +#define PTRAUTH_REG_OFFSET(x)	(x - CPU_APIAKEYLO_EL1)
> +
> +/*
> + * CPU_AP*_EL1 values exceed immediate offset range (512) for stp instruction
> + * so below macros takes CPU_APIAKEYLO_EL1 as base and calculates the offset of
> + * the keys from this base to avoid an extra add instruction. These macros
> + * assumes the keys offsets are aligned in a specific increasing order.
> + */
> +.macro	ptrauth_save_state base, reg1, reg2
> +	mrs_s	\reg1, SYS_APIAKEYLO_EL1
> +	mrs_s	\reg2, SYS_APIAKEYHI_EL1
> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
> +	mrs_s	\reg1, SYS_APIBKEYLO_EL1
> +	mrs_s	\reg2, SYS_APIBKEYHI_EL1
> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
> +	mrs_s	\reg1, SYS_APDAKEYLO_EL1
> +	mrs_s	\reg2, SYS_APDAKEYHI_EL1
> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
> +	mrs_s	\reg1, SYS_APDBKEYLO_EL1
> +	mrs_s	\reg2, SYS_APDBKEYHI_EL1
> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
> +	mrs_s	\reg1, SYS_APGAKEYLO_EL1
> +	mrs_s	\reg2, SYS_APGAKEYHI_EL1
> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
> +.endm
> +
> +.macro	ptrauth_restore_state base, reg1, reg2
> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
> +	msr_s	SYS_APIAKEYLO_EL1, \reg1
> +	msr_s	SYS_APIAKEYHI_EL1, \reg2
> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
> +	msr_s	SYS_APIBKEYLO_EL1, \reg1
> +	msr_s	SYS_APIBKEYHI_EL1, \reg2
> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
> +	msr_s	SYS_APDAKEYLO_EL1, \reg1
> +	msr_s	SYS_APDAKEYHI_EL1, \reg2
> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
> +	msr_s	SYS_APDBKEYLO_EL1, \reg1
> +	msr_s	SYS_APDBKEYHI_EL1, \reg2
> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
> +	msr_s	SYS_APGAKEYLO_EL1, \reg1
> +	msr_s	SYS_APGAKEYHI_EL1, \reg2
> +.endm
> +
> +/* Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will check for

Comment style.

> + * the presence of one of the cpufeature flag ARM64_HAS_ADDRESS_AUTH_ARCH or
> + * ARM64_HAS_ADDRESS_AUTH_IMP_DEF and then proceed ahead with the save/restore
> + * of Pointer Authentication key registers.
> + */
> +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
> +alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
> +	b	1000f
> +alternative_else_nop_endif
> +alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
> +	b	1001f
> +alternative_else_nop_endif
> +1000:
> +	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
> +	and	\reg1, \reg1, #(HCR_API | HCR_APK)
> +	cbz	\reg1, 1001f
> +	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
> +	ptrauth_restore_state	\reg1, \reg2, \reg3
> +1001:
> +.endm
> +
> +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
> +alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
> +	b	2000f
> +alternative_else_nop_endif
> +alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
> +	b	2001f
> +alternative_else_nop_endif
> +2000:
> +	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
> +	and	\reg1, \reg1, #(HCR_API | HCR_APK)
> +	cbz	\reg1, 2001f
> +	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
> +	ptrauth_save_state	\reg1, \reg2, \reg3
> +	add	\reg1, \h_ctxt, #CPU_APIAKEYLO_EL1
> +	ptrauth_restore_state	\reg1, \reg2, \reg3
> +	isb
> +2001:
> +.endm
> +
> +#else /* !CONFIG_ARM64_PTR_AUTH */
> +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
> +.endm
> +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
> +.endm
> +#endif /* CONFIG_ARM64_PTR_AUTH */
> +#endif /* __ASSEMBLY__ */
> +#endif /* __ASM_KVM_PTRAUTH_H */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 7f40dcb..8178330 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -125,7 +125,13 @@ int main(void)
>    DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
>    DEFINE(VCPU_FAULT_DISR,	offsetof(struct kvm_vcpu, arch.fault.disr_el1));
>    DEFINE(VCPU_WORKAROUND_FLAGS,	offsetof(struct kvm_vcpu, arch.workaround_flags));
> +  DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(CPU_GP_REGS,		offsetof(struct kvm_cpu_context, gp_regs));
> +  DEFINE(CPU_APIAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIAKEYLO_EL1]));
> +  DEFINE(CPU_APIBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIBKEYLO_EL1]));
> +  DEFINE(CPU_APDAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDAKEYLO_EL1]));
> +  DEFINE(CPU_APDBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDBKEYLO_EL1]));
> +  DEFINE(CPU_APGAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APGAKEYLO_EL1]));
>    DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_regs, regs));
>    DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
>  #endif
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 0b79834..516aead 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -173,20 +173,40 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +#define __ptrauth_save_key(regs, key)						\
> +({										\
> +	regs[key ## KEYLO_EL1] = read_sysreg_s(SYS_ ## key ## KEYLO_EL1);	\
> +	regs[key ## KEYHI_EL1] = read_sysreg_s(SYS_ ## key ## KEYHI_EL1);	\
> +})
> +
> +/*
> + * 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)
> +{
> +	struct kvm_cpu_context *ctxt;
> +
> +	if (vcpu_has_ptrauth(vcpu)) {
> +		vcpu_ptrauth_enable(vcpu);
> +		ctxt = vcpu->arch.host_cpu_context;
> +		__ptrauth_save_key(ctxt->sys_regs, APIA);
> +		__ptrauth_save_key(ctxt->sys_regs, APIB);
> +		__ptrauth_save_key(ctxt->sys_regs, APDA);
> +		__ptrauth_save_key(ctxt->sys_regs, APDB);
> +		__ptrauth_save_key(ctxt->sys_regs, APGA);
> +	} else {
> +		kvm_inject_undefined(vcpu);
> +	}
> +}
> +
>  /*
>   * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
>   * a NOP).
>   */
>  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/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 675fdc1..93ba3d7 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -24,6 +24,7 @@
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> +#include <asm/kvm_ptrauth.h>
>  
>  #define CPU_GP_REG_OFFSET(x)	(CPU_GP_REGS + x)
>  #define CPU_XREG_OFFSET(x)	CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
> @@ -64,6 +65,13 @@ ENTRY(__guest_enter)
>  
>  	add	x18, x0, #VCPU_CONTEXT
>  
> +	// Macro ptrauth_switch_to_guest format:
> +	// 	ptrauth_switch_to_guest(guest cxt, tmp1, tmp2, tmp3)
> +	// The below macro to restore guest keys is not implemented in C code
> +	// as it may cause Pointer Authentication key signing mismatch errors
> +	// when this feature is enabled for kernel code.
> +	ptrauth_switch_to_guest x18, x0, x1, x2
> +
>  	// Restore guest regs x0-x17
>  	ldp	x0, x1,   [x18, #CPU_XREG_OFFSET(0)]
>  	ldp	x2, x3,   [x18, #CPU_XREG_OFFSET(2)]
> @@ -118,6 +126,13 @@ ENTRY(__guest_exit)
>  
>  	get_host_ctxt	x2, x3
>  
> +	// Macro ptrauth_switch_to_guest format:
> +	// 	ptrauth_switch_to_host(guest cxt, host cxt, tmp1, tmp2, tmp3)
> +	// The below macro to save/restore keys is not implemented in C code
> +	// as it may cause Pointer Authentication key signing mismatch errors
> +	// when this feature is enabled for kernel code.
> +	ptrauth_switch_to_host x1, x2, x3, x4, x5
> +
>  	// Now restore the host regs
>  	restore_callee_saved_regs x2
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7046c76..7f06c2e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1007,6 +1007,35 @@ 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), }
>  
> +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 is false for both cases and PC remains same,
> +	 * a) Re-execute the same key register access instruction after enabling
> +	 *    ptrauth.
> +	 * b) UNDEF is injected as ptrauth is not supported/enabled.
> +	 */

This comment is pretty confusing. You say that PC remains the same,
but that's obviously not true for (b). I've fixed it locally as such:

	/*
	 * Return false for both cases as we never skip the trapped
	 * instruction:
	 *
	 * - Either we re-execute the same key register access instruction
	 *   after enabling ptrauth.
	 * - Or an UNDEF is injected as ptrauth is not supported/enabled.
	 */


> +	return false;
> +}
> +
> +static unsigned int ptrauth_visibility(const struct kvm_vcpu *vcpu,
> +			const struct sys_reg_desc *rd)
> +{
> +	return vcpu_has_ptrauth(vcpu) ? 0 : REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> +}
> +
> +#define __PTRAUTH_KEY(k)						\
> +	{ SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k,		\
> +	.visibility = ptrauth_visibility}
> +
> +#define PTRAUTH_KEY(k)							\
> +	__PTRAUTH_KEY(k ## KEYLO_EL1),					\
> +	__PTRAUTH_KEY(k ## KEYHI_EL1)
> +
>  static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  			      struct sys_reg_params *p,
>  			      const struct sys_reg_desc *r)
> @@ -1058,9 +1087,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  					 (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 (!vcpu_has_ptrauth(vcpu)) {
> +			if (val & ptrauth_mask)
> +				kvm_debug("ptrauth unsupported for guests, suppressing\n");
> +			val &= ~ptrauth_mask;
> +		}
>  	}
>  
>  	return val;
> @@ -1460,6 +1491,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 7039c99c..156c09d 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -385,6 +385,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		vcpu_clear_wfe_traps(vcpu);
>  	else
>  		vcpu_set_wfe_traps(vcpu);
> +
> +	vcpu_ptrauth_setup_lazy(vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> -- 
> 2.7.4
> 

I've tentatively applied these fixes on top of your series, no need to
resend anything for now.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23  9:39   ` Marc Zyngier
@ 2019-04-23 10:24     ` Amit Daniel Kachhap
  2019-04-23 15:44       ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-23 10:24 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-arm-kernel, Christoffer Dall, Catalin Marinas, Will Deacon,
	Andrew Jones, Dave Martin, Ramana Radhakrishnan, kvmarm,
	Kristina Martsenko, linux-kernel, Mark Rutland, James Morse,
	Julien Thierry

Hi Mark,

On 4/23/19 3:09 PM, Marc Zyngier wrote:
> On Tue, 23 Apr 2019 05:42:35 +0100,
> Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>>
>> From: Mark Rutland <mark.rutland@arm.com>
>>
>> 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
>> in the kernel and present in the 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. However the host key save is
>> optimized and implemented inside ptrauth instruction/register access
>> trap.
>>
>> 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). Hence, this patch expects both type of
>> authentication to be present in a cpu.
>>
>> This switch of key is done from guest enter/exit assembly as preparation
>> for the upcoming in-kernel pointer authentication support. Hence, these
>> key switching routines are not implemented in C code as they may cause
>> pointer authentication key signing error in some situations.
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
>> , save host key in ptrauth exception trap]
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>> Cc: kvmarm@lists.cs.columbia.edu
>> ---
>> Changes since v9:
>>
>> * Removed hardcoding of enum values[Mark Zyngier].
>> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
>> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
>> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
>>    kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
>> * Added/Modified comments at few places [Marc Zyngier].
>>
>>   arch/arm/include/asm/kvm_emulate.h   |   2 +
>>   arch/arm64/Kconfig                   |   5 +-
>>   arch/arm64/include/asm/kvm_emulate.h |  16 ++++++
>>   arch/arm64/include/asm/kvm_host.h    |  14 +++++
>>   arch/arm64/include/asm/kvm_ptrauth.h | 108 +++++++++++++++++++++++++++++++++++
>>   arch/arm64/kernel/asm-offsets.c      |   6 ++
>>   arch/arm64/kvm/handle_exit.c         |  36 +++++++++---
>>   arch/arm64/kvm/hyp/entry.S           |  15 +++++
>>   arch/arm64/kvm/sys_regs.c            |  43 +++++++++++++-
>>   virt/kvm/arm/arm.c                   |   2 +
>>   10 files changed, 234 insertions(+), 13 deletions(-)
>>   create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index 8927cae..efb0e2c 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -343,4 +343,6 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>   	}
>>   }
>>   
>> +static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu) {}
>> +
>>   #endif /* __ARM_KVM_EMULATE_H__ */
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 7e34b9e..3cfe2eb 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
>>   	  context-switched along with the process.
>>   
>>   	  The feature is detected at runtime. If the feature is not present in
>> -	  hardware it will not be advertised to userspace nor will it be
>> -	  enabled.
>> +	  hardware it will not be advertised to userspace/KVM guest nor will it
>> +	  be enabled. However, KVM guest also require VHE mode and hence
>> +	  CONFIG_ARM64_VHE=y option to use this feature.
> 
> SVE seems to have the exact same requirements, and has
> 
> 	depends on !KVM || ARM64_VHE
> 
> Why don't we have that for PTR_AUTH too?
This point came up earlier also and it was suggested by  Dave[1] to 
leave userspace ptrauth for non-vhe mode as that would bring regression now.
[1]:https://lkml.org/lkml/2019/3/27/583
> 
>>   
>>   endmenu
>>   
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index d384279..613427f 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -98,6 +98,22 @@ static inline void vcpu_set_wfe_traps(struct kvm_vcpu *vcpu)
>>   	vcpu->arch.hcr_el2 |= HCR_TWE;
>>   }
>>   
>> +static inline void vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcr_el2 |= (HCR_API | HCR_APK);
>> +}
>> +
>> +static inline void vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcr_el2 &= ~(HCR_API | HCR_APK);
>> +}
>> +
>> +static inline void vcpu_ptrauth_setup_lazy(struct kvm_vcpu *vcpu)
>> +{
>> +	if (vcpu_has_ptrauth(vcpu))
>> +		vcpu_ptrauth_disable(vcpu);
>> +}
>> +
>>   static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
>>   {
>>   	return vcpu->arch.vsesr_el2;
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7ccac42..7eebea7 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -161,6 +161,18 @@ enum vcpu_sysreg {
>>   	PMSWINC_EL0,	/* Software Increment Register */
>>   	PMUSERENR_EL0,	/* User Enable Register */
>>   
>> +	/* Pointer Authentication Registers in a strict increasing order. */
>> +	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 */
>> @@ -530,6 +542,8 @@ static inline bool kvm_arch_requires_vhe(void)
>>   	return false;
>>   }
>>   
>> +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_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
>> diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h
>> new file mode 100644
>> index 0000000..f337237
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/kvm_ptrauth.h
>> @@ -0,0 +1,108 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* arch/arm64/include/asm/kvm_ptrauth.h: Guest/host ptrauth save/restore
>> + * Copyright 2019 Arm Limited
>> + * Authors: Mark Rutland <mark.rutland@arm.com>
>> + *         Amit Daniel Kachhap <amit.kachhap@arm.com>
>> + */
>> +
>> +#ifndef __ASM_KVM_PTRAUTH_H
>> +#define __ASM_KVM_PTRAUTH_H
>> +
>> +#ifdef __ASSEMBLY__
>> +
>> +#include <asm/sysreg.h>
>> +
>> +#ifdef	CONFIG_ARM64_PTR_AUTH
>> +
>> +#define PTRAUTH_REG_OFFSET(x)	(x - CPU_APIAKEYLO_EL1)
>> +
>> +/*
>> + * CPU_AP*_EL1 values exceed immediate offset range (512) for stp instruction
>> + * so below macros takes CPU_APIAKEYLO_EL1 as base and calculates the offset of
>> + * the keys from this base to avoid an extra add instruction. These macros
>> + * assumes the keys offsets are aligned in a specific increasing order.
>> + */
>> +.macro	ptrauth_save_state base, reg1, reg2
>> +	mrs_s	\reg1, SYS_APIAKEYLO_EL1
>> +	mrs_s	\reg2, SYS_APIAKEYHI_EL1
>> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
>> +	mrs_s	\reg1, SYS_APIBKEYLO_EL1
>> +	mrs_s	\reg2, SYS_APIBKEYHI_EL1
>> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
>> +	mrs_s	\reg1, SYS_APDAKEYLO_EL1
>> +	mrs_s	\reg2, SYS_APDAKEYHI_EL1
>> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
>> +	mrs_s	\reg1, SYS_APDBKEYLO_EL1
>> +	mrs_s	\reg2, SYS_APDBKEYHI_EL1
>> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
>> +	mrs_s	\reg1, SYS_APGAKEYLO_EL1
>> +	mrs_s	\reg2, SYS_APGAKEYHI_EL1
>> +	stp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
>> +.endm
>> +
>> +.macro	ptrauth_restore_state base, reg1, reg2
>> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIAKEYLO_EL1)]
>> +	msr_s	SYS_APIAKEYLO_EL1, \reg1
>> +	msr_s	SYS_APIAKEYHI_EL1, \reg2
>> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APIBKEYLO_EL1)]
>> +	msr_s	SYS_APIBKEYLO_EL1, \reg1
>> +	msr_s	SYS_APIBKEYHI_EL1, \reg2
>> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDAKEYLO_EL1)]
>> +	msr_s	SYS_APDAKEYLO_EL1, \reg1
>> +	msr_s	SYS_APDAKEYHI_EL1, \reg2
>> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APDBKEYLO_EL1)]
>> +	msr_s	SYS_APDBKEYLO_EL1, \reg1
>> +	msr_s	SYS_APDBKEYHI_EL1, \reg2
>> +	ldp	\reg1, \reg2, [\base, #PTRAUTH_REG_OFFSET(CPU_APGAKEYLO_EL1)]
>> +	msr_s	SYS_APGAKEYLO_EL1, \reg1
>> +	msr_s	SYS_APGAKEYHI_EL1, \reg2
>> +.endm
>> +
>> +/* Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will check for
> 
> Comment style.
> 
>> + * the presence of one of the cpufeature flag ARM64_HAS_ADDRESS_AUTH_ARCH or
>> + * ARM64_HAS_ADDRESS_AUTH_IMP_DEF and then proceed ahead with the save/restore
>> + * of Pointer Authentication key registers.
>> + */
>> +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
>> +alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
>> +	b	1000f
>> +alternative_else_nop_endif
>> +alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
>> +	b	1001f
>> +alternative_else_nop_endif
>> +1000:
>> +	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
>> +	and	\reg1, \reg1, #(HCR_API | HCR_APK)
>> +	cbz	\reg1, 1001f
>> +	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
>> +	ptrauth_restore_state	\reg1, \reg2, \reg3
>> +1001:
>> +.endm
>> +
>> +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
>> +alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
>> +	b	2000f
>> +alternative_else_nop_endif
>> +alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
>> +	b	2001f
>> +alternative_else_nop_endif
>> +2000:
>> +	ldr	\reg1, [\g_ctxt, #(VCPU_HCR_EL2 - VCPU_CONTEXT)]
>> +	and	\reg1, \reg1, #(HCR_API | HCR_APK)
>> +	cbz	\reg1, 2001f
>> +	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
>> +	ptrauth_save_state	\reg1, \reg2, \reg3
>> +	add	\reg1, \h_ctxt, #CPU_APIAKEYLO_EL1
>> +	ptrauth_restore_state	\reg1, \reg2, \reg3
>> +	isb
>> +2001:
>> +.endm
>> +
>> +#else /* !CONFIG_ARM64_PTR_AUTH */
>> +.macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
>> +.endm
>> +.macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3
>> +.endm
>> +#endif /* CONFIG_ARM64_PTR_AUTH */
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* __ASM_KVM_PTRAUTH_H */
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 7f40dcb..8178330 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -125,7 +125,13 @@ int main(void)
>>     DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
>>     DEFINE(VCPU_FAULT_DISR,	offsetof(struct kvm_vcpu, arch.fault.disr_el1));
>>     DEFINE(VCPU_WORKAROUND_FLAGS,	offsetof(struct kvm_vcpu, arch.workaround_flags));
>> +  DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>>     DEFINE(CPU_GP_REGS,		offsetof(struct kvm_cpu_context, gp_regs));
>> +  DEFINE(CPU_APIAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIAKEYLO_EL1]));
>> +  DEFINE(CPU_APIBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIBKEYLO_EL1]));
>> +  DEFINE(CPU_APDAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDAKEYLO_EL1]));
>> +  DEFINE(CPU_APDBKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APDBKEYLO_EL1]));
>> +  DEFINE(CPU_APGAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APGAKEYLO_EL1]));
>>     DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_regs, regs));
>>     DEFINE(HOST_CONTEXT_VCPU,	offsetof(struct kvm_cpu_context, __hyp_running_vcpu));
>>   #endif
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 0b79834..516aead 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -173,20 +173,40 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>   	return 1;
>>   }
>>   
>> +#define __ptrauth_save_key(regs, key)						\
>> +({										\
>> +	regs[key ## KEYLO_EL1] = read_sysreg_s(SYS_ ## key ## KEYLO_EL1);	\
>> +	regs[key ## KEYHI_EL1] = read_sysreg_s(SYS_ ## key ## KEYHI_EL1);	\
>> +})
>> +
>> +/*
>> + * 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)
>> +{
>> +	struct kvm_cpu_context *ctxt;
>> +
>> +	if (vcpu_has_ptrauth(vcpu)) {
>> +		vcpu_ptrauth_enable(vcpu);
>> +		ctxt = vcpu->arch.host_cpu_context;
>> +		__ptrauth_save_key(ctxt->sys_regs, APIA);
>> +		__ptrauth_save_key(ctxt->sys_regs, APIB);
>> +		__ptrauth_save_key(ctxt->sys_regs, APDA);
>> +		__ptrauth_save_key(ctxt->sys_regs, APDB);
>> +		__ptrauth_save_key(ctxt->sys_regs, APGA);
>> +	} else {
>> +		kvm_inject_undefined(vcpu);
>> +	}
>> +}
>> +
>>   /*
>>    * Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
>>    * a NOP).
>>    */
>>   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/entry.S b/arch/arm64/kvm/hyp/entry.S
>> index 675fdc1..93ba3d7 100644
>> --- a/arch/arm64/kvm/hyp/entry.S
>> +++ b/arch/arm64/kvm/hyp/entry.S
>> @@ -24,6 +24,7 @@
>>   #include <asm/kvm_arm.h>
>>   #include <asm/kvm_asm.h>
>>   #include <asm/kvm_mmu.h>
>> +#include <asm/kvm_ptrauth.h>
>>   
>>   #define CPU_GP_REG_OFFSET(x)	(CPU_GP_REGS + x)
>>   #define CPU_XREG_OFFSET(x)	CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
>> @@ -64,6 +65,13 @@ ENTRY(__guest_enter)
>>   
>>   	add	x18, x0, #VCPU_CONTEXT
>>   
>> +	// Macro ptrauth_switch_to_guest format:
>> +	// 	ptrauth_switch_to_guest(guest cxt, tmp1, tmp2, tmp3)
>> +	// The below macro to restore guest keys is not implemented in C code
>> +	// as it may cause Pointer Authentication key signing mismatch errors
>> +	// when this feature is enabled for kernel code.
>> +	ptrauth_switch_to_guest x18, x0, x1, x2
>> +
>>   	// Restore guest regs x0-x17
>>   	ldp	x0, x1,   [x18, #CPU_XREG_OFFSET(0)]
>>   	ldp	x2, x3,   [x18, #CPU_XREG_OFFSET(2)]
>> @@ -118,6 +126,13 @@ ENTRY(__guest_exit)
>>   
>>   	get_host_ctxt	x2, x3
>>   
>> +	// Macro ptrauth_switch_to_guest format:
>> +	// 	ptrauth_switch_to_host(guest cxt, host cxt, tmp1, tmp2, tmp3)
>> +	// The below macro to save/restore keys is not implemented in C code
>> +	// as it may cause Pointer Authentication key signing mismatch errors
>> +	// when this feature is enabled for kernel code.
>> +	ptrauth_switch_to_host x1, x2, x3, x4, x5
>> +
>>   	// Now restore the host regs
>>   	restore_callee_saved_regs x2
>>   
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 7046c76..7f06c2e 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1007,6 +1007,35 @@ 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), }
>>   
>> +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 is false for both cases and PC remains same,
>> +	 * a) Re-execute the same key register access instruction after enabling
>> +	 *    ptrauth.
>> +	 * b) UNDEF is injected as ptrauth is not supported/enabled.
>> +	 */
> 
> This comment is pretty confusing. You say that PC remains the same,
> but that's obviously not true for (b). I've fixed it locally as such:
> 
> 	/*
> 	 * Return false for both cases as we never skip the trapped
> 	 * instruction:
> 	 *
> 	 * - Either we re-execute the same key register access instruction
> 	 *   after enabling ptrauth.
> 	 * - Or an UNDEF is injected as ptrauth is not supported/enabled.
> 	 */
> 
ok.
> 
>> +	return false;
>> +}
>> +
>> +static unsigned int ptrauth_visibility(const struct kvm_vcpu *vcpu,
>> +			const struct sys_reg_desc *rd)
>> +{
>> +	return vcpu_has_ptrauth(vcpu) ? 0 : REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>> +}
>> +
>> +#define __PTRAUTH_KEY(k)						\
>> +	{ SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k,		\
>> +	.visibility = ptrauth_visibility}
>> +
>> +#define PTRAUTH_KEY(k)							\
>> +	__PTRAUTH_KEY(k ## KEYLO_EL1),					\
>> +	__PTRAUTH_KEY(k ## KEYHI_EL1)
>> +
>>   static bool access_arch_timer(struct kvm_vcpu *vcpu,
>>   			      struct sys_reg_params *p,
>>   			      const struct sys_reg_desc *r)
>> @@ -1058,9 +1087,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>   					 (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 (!vcpu_has_ptrauth(vcpu)) {
>> +			if (val & ptrauth_mask)
>> +				kvm_debug("ptrauth unsupported for guests, suppressing\n");
>> +			val &= ~ptrauth_mask;
>> +		}
>>   	}
>>   
>>   	return val;
>> @@ -1460,6 +1491,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 7039c99c..156c09d 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -385,6 +385,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>   		vcpu_clear_wfe_traps(vcpu);
>>   	else
>>   		vcpu_set_wfe_traps(vcpu);
>> +
>> +	vcpu_ptrauth_setup_lazy(vcpu);
>>   }
>>   
>>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> -- 
>> 2.7.4
>>
> 
> I've tentatively applied these fixes on top of your series, no need to
> resend anything for now.

Thanks,
Amit D.
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest
  2019-04-23  4:42 ` [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest Amit Daniel Kachhap
@ 2019-04-23 15:44   ` Dave Martin
  2019-04-24  5:57     ` Amit Daniel Kachhap
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-23 15:44 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Tue, Apr 23, 2019 at 10:12:34AM +0530, Amit Daniel Kachhap wrote:
> A per vcpu flag is added to check if pointer authentication is
> enabled for the vcpu or not. This flag may be enabled according to
> the necessary user policies and host capabilities.
> 
> This patch also adds a helper to check the flag.
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> 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: kvmarm@lists.cs.columbia.edu
> ---
> Changes since v9:
> 
> * Added ptrauth cpufeature static check in vcpu_has_ptrauth [Marc Zyngier].
> 
>  arch/arm64/include/asm/kvm_host.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7a096fd..7ccac42 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -355,10 +355,15 @@ struct kvm_vcpu_arch {
>  #define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
>  #define KVM_ARM64_GUEST_HAS_SVE		(1 << 5) /* SVE exposed to guest */
>  #define KVM_ARM64_VCPU_SVE_FINALIZED	(1 << 6) /* SVE config completed */
> +#define KVM_ARM64_GUEST_HAS_PTRAUTH	(1 << 7) /* PTRAUTH exposed to guest */
>  
>  #define vcpu_has_sve(vcpu) (system_supports_sve() && \
>  			    ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_SVE))
>  
> +#define vcpu_has_ptrauth(vcpu)	((system_supports_address_auth() || \
> +				  system_supports_generic_auth()) && \

Come to think of it, should this be
system_supports_address_auth() _&&_ system_supports_generic_auth()?

It won't make a functional difference today though, since today
kvm_vcpu_enable_ptrauth() won't set KVM_ARM64_GUEST_HAS_PTRAUTH without
system_supports_address_auth() and system_supports_generic_auth() both
true.

With || here, we won't have to change this if supporting the two auth
types independently in the future though.

Either way, my Reviewed-by stands.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23 10:24     ` Amit Daniel Kachhap
@ 2019-04-23 15:44       ` Dave Martin
  2019-04-24 10:29         ` Marc Zyngier
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-23 15:44 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: Marc Zyngier, linux-kernel, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan,
	linux-arm-kernel

On Tue, Apr 23, 2019 at 03:54:32PM +0530, Amit Daniel Kachhap wrote:
> Hi Mark,
> 
> On 4/23/19 3:09 PM, Marc Zyngier wrote:
> >On Tue, 23 Apr 2019 05:42:35 +0100,
> >Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
> >>
> >>From: Mark Rutland <mark.rutland@arm.com>
> >>
> >>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
> >>in the kernel and present in the 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. However the host key save is
> >>optimized and implemented inside ptrauth instruction/register access
> >>trap.
> >>
> >>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). Hence, this patch expects both type of
> >>authentication to be present in a cpu.
> >>
> >>This switch of key is done from guest enter/exit assembly as preparation
> >>for the upcoming in-kernel pointer authentication support. Hence, these
> >>key switching routines are not implemented in C code as they may cause
> >>pointer authentication key signing error in some situations.
> >>
> >>Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >>[Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
> >>, save host key in ptrauth exception trap]
> >>Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> >>Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> >>Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>Cc: Christoffer Dall <christoffer.dall@arm.com>
> >>Cc: kvmarm@lists.cs.columbia.edu
> >>---
> >>Changes since v9:
> >>
> >>* Removed hardcoding of enum values[Mark Zyngier].
> >>* Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
> >>* Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
> >>* Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
> >>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
> >>* Added/Modified comments at few places [Marc Zyngier].
> >>
> >>  arch/arm/include/asm/kvm_emulate.h   |   2 +
> >>  arch/arm64/Kconfig                   |   5 +-
> >>  arch/arm64/include/asm/kvm_emulate.h |  16 ++++++
> >>  arch/arm64/include/asm/kvm_host.h    |  14 +++++
> >>  arch/arm64/include/asm/kvm_ptrauth.h | 108 +++++++++++++++++++++++++++++++++++
> >>  arch/arm64/kernel/asm-offsets.c      |   6 ++
> >>  arch/arm64/kvm/handle_exit.c         |  36 +++++++++---
> >>  arch/arm64/kvm/hyp/entry.S           |  15 +++++
> >>  arch/arm64/kvm/sys_regs.c            |  43 +++++++++++++-
> >>  virt/kvm/arm/arm.c                   |   2 +
> >>  10 files changed, 234 insertions(+), 13 deletions(-)
> >>  create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h

[...]

> >>diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>index 7e34b9e..3cfe2eb 100644
> >>--- a/arch/arm64/Kconfig
> >>+++ b/arch/arm64/Kconfig
> >>@@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
> >>  	  context-switched along with the process.
> >>  	  The feature is detected at runtime. If the feature is not present in
> >>-	  hardware it will not be advertised to userspace nor will it be
> >>-	  enabled.
> >>+	  hardware it will not be advertised to userspace/KVM guest nor will it
> >>+	  be enabled. However, KVM guest also require VHE mode and hence
> >>+	  CONFIG_ARM64_VHE=y option to use this feature.
> >
> >SVE seems to have the exact same requirements, and has
> >
> >	depends on !KVM || ARM64_VHE
> >
> >Why don't we have that for PTR_AUTH too?
> This point came up earlier also and it was suggested by  Dave[1] to leave
> userspace ptrauth for non-vhe mode as that would bring regression now.
> [1]:https://lkml.org/lkml/2019/3/27/583

I see Marc applied this change in
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=e19b245fa4c61558536bd34f80845f0c41eab65f0

The risk here is that someone has a custom config from an old kernel
that explicitly turns CONFIG_ARM64_VHE off, and that try to use that
config with this patch.

I'm not sure how much we care about that.

Otherwise, blocking this config so that people don't accidentally rely
on it seems sensible.

[...]

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication
  2019-04-23  4:42 ` [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication Amit Daniel Kachhap
@ 2019-04-23 15:45   ` Dave Martin
  2019-04-24  6:39     ` Amit Daniel Kachhap
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-23 15:45 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Tue, Apr 23, 2019 at 10:12:36AM +0530, Amit Daniel Kachhap wrote:
> Now that the building blocks of pointer authentication are present, lets
> add userspace flags KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> KVM_ARM_VCPU_PTRAUTH_GENERIC. These flags will enable pointer
> authentication for the KVM guest on a per-vcpu basis through the ioctl
> KVM_ARM_VCPU_INIT.
> 
> This features will allow the KVM guest to allow the handling of
> pointer authentication instructions or to treat them as undefined
> if not set.
> 
> Necessary documentations are added to reflect the changes done.
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> 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: kvmarm@lists.cs.columbia.edu
> ---
> Changed since v9:
> * Fixed tab alignment at few places [Dave Martin].
> * Split the system capability checks [Dave Martin].
> 
>  Documentation/arm64/pointer-authentication.txt | 22 +++++++++++++++++----
>  Documentation/virtual/kvm/api.txt              | 10 ++++++++++
>  arch/arm64/include/asm/kvm_host.h              |  2 +-
>  arch/arm64/include/uapi/asm/kvm.h              |  2 ++
>  arch/arm64/kvm/reset.c                         | 27 ++++++++++++++++++++++++++
>  5 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
> index 5baca42..fc71b33 100644
> --- a/Documentation/arm64/pointer-authentication.txt
> +++ b/Documentation/arm64/pointer-authentication.txt
> @@ -87,7 +87,21 @@ used to get and set the keys for a thread.
>  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 each virtual cpu is
> +initialised by passing flags KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] and
> +requesting these two separate cpu features to be enabled. The current KVM
> +guest implementation works by enabling both features together, so both
> +these userspace flags are checked before enabling pointer authentication.
> +The separate userspace flag will allow to have no userspace ABI changes
> +if support is added in the future to allow these two features to be
> +enabled independently of one another.
> +
> +As Arm Architecture specifies that Pointer Authentication feature is
> +implemented along with the VHE feature so KVM arm64 ptrauth code relies
> +on VHE mode to be present.
> +
> +Additionally, when these vcpu feature flags are not set then KVM will
> +filter out the Pointer Authentication system key registers from
> +KVM_GET/SET_REG_* ioctls and mask those features from cpufeature ID
> +register. Any attempt to use the Pointer Authentication instructions 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 e410a9f..32afe7f 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2761,6 +2761,16 @@ Possible features:
>  	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
>  	  Depends on KVM_CAP_ARM_PMU_V3.
>  
> +	- KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
> +	  for arm64 only.
> +	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
> +	  must be requested or neither must be requested.
> +
> +	- KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
> +	  for arm64 only.
> +	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
> +	  must be requested or neither must be requested.
> +

This looks pretty clear now.

>  	- KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only).
>  	  Depends on KVM_CAP_ARM_SVE.
>  	  Requires KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE):
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7eebea7..f772ac2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -49,7 +49,7 @@
>  
>  #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>  
> -#define KVM_VCPU_MAX_FEATURES 5
> +#define KVM_VCPU_MAX_FEATURES 7
>  
>  #define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index edd2db8..7b7ac0f 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -104,6 +104,8 @@ 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 */
>  #define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
> +#define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
> +#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
>  
>  struct kvm_vcpu_init {
>  	__u32 target;
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 3402543..028d0c6 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -221,6 +221,27 @@ static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
>  		memset(vcpu->arch.sve_state, 0, vcpu_sve_state_size(vcpu));
>  }
>  
> +static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> +{
> +	/* Support ptrauth only if the system supports these capabilities. */
> +	if (!has_vhe())
> +		return -EINVAL;
> +
> +	if (!system_supports_address_auth() ||
> +	    !system_supports_generic_auth())
> +		return -EINVAL;

Since pointer auth implies v8.3 and v8.3 implies v8.1 and v8.1 implies VHE:

((system_supports_address_auth() || system_supports_generic_auth()) &&
!has_vhe()) implies that the hardware is broken or the kernel is buggy.

So, it probably makes sense to write

	if (!system_supports_address_auth() ||
	    !system_supports_generic_auth())
		return -EINVAL;

	if (WARN_ON(!has_vhe()))
		return -EINVAL;

This is not essential, and doesn't affect the ABI -- so we could apply
it on top later on.

[...]

FWIW:

Reviewed-by: Dave Martin <Dave.Martin@arm.com> (for the updates)

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest
  2019-04-23  4:42 ` [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest Amit Daniel Kachhap
@ 2019-04-23 15:45   ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2019-04-23 15:45 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Tue, Apr 23, 2019 at 10:12:37AM +0530, Amit Daniel Kachhap wrote:
> This patch advertises the capability of two cpu feature called address
> pointer authentication and generic pointer authentication. These
> capabilities depend upon system support for pointer authentication and
> VHE mode.
> 
> The current arm64 KVM partially implements pointer authentication and
> support of address/generic authentication are tied together. However,
> separate ABI requirements for both of them is added so that any future
> isolated implementation will not require any ABI changes.
> 
> 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: kvmarm@lists.cs.columbia.edu
> ---
> Changes since v9:
> * Fixed tab alignment issues [Dave Martin].
> * Clarify the api documentation [Dave Martin].
> 
>  Documentation/virtual/kvm/api.txt | 14 ++++++++++----
>  arch/arm64/kvm/reset.c            |  5 +++++
>  include/uapi/linux/kvm.h          |  2 ++
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 32afe7f..fac1887 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2763,13 +2763,19 @@ Possible features:
>  
>  	- KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>  	  for arm64 only.
> -	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
> -	  must be requested or neither must be requested.
> +	  Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
> +	  If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> +	  both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> +	  KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> +	  requested.
>  
>  	- KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>  	  for arm64 only.
> -	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
> -	  must be requested or neither must be requested.
> +	  Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
> +	  If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> +	  both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> +	  KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> +	  requested.

This looks clear now.

The description is quite heavyweight, but should be hard for people to
misunderstand.

>  	- KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only).
>  	  Depends on KVM_CAP_ARM_SVE.
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 028d0c6..f0faf54 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -101,6 +101,11 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_SVE:
>  		r = system_supports_sve();
>  		break;
> +	case KVM_CAP_ARM_PTRAUTH_ADDRESS:
> +	case KVM_CAP_ARM_PTRAUTH_GENERIC:
> +		r = has_vhe() && system_supports_address_auth() &&
> +				 system_supports_generic_auth();
> +		break;
>  	default:
>  		r = 0;
>  	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 1d56444..4dc34f8 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -989,6 +989,8 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
>  #define KVM_CAP_HYPERV_CPUID 167
>  #define KVM_CAP_ARM_SVE 168
> +#define KVM_CAP_ARM_PTRAUTH_ADDRESS 169
> +#define KVM_CAP_ARM_PTRAUTH_GENERIC 170

[...]

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-04-23  4:42 ` [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
@ 2019-04-23 15:46   ` Dave Martin
  2019-04-24  7:02     ` Amit Daniel Kachhap
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-23 15:46 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Tue, Apr 23, 2019 at 10:12:38AM +0530, Amit Daniel Kachhap wrote:
> This patch adds a runtime capabality for KVM tool to enable Arm64 8.3
> Pointer Authentication in guest kernel. Two vcpu features
> KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable
> Pointer Authentication in KVM guest after checking the capability.
> 
> Command line options --enable-ptrauth and --disable-ptrauth are added
> to use this feature. However, if those options are not provided then
> also this feature is enabled if host supports this capability.
> 
> The macros defined in the headers are not in sync and should be replaced
> from the upstream.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
> 
> Changes since v9:
> * Added a error check for both enable-ptrauth and disable-ptrauth
>   option.
> * Make the error explicit when enable-ptrauth is provided [Dave Martin].
> 
>  arm/aarch32/include/kvm/kvm-cpu-arch.h    |  1 +
>  arm/aarch64/include/asm/kvm.h             |  2 ++
>  arm/aarch64/include/kvm/kvm-config-arch.h |  6 +++++-
>  arm/aarch64/include/kvm/kvm-cpu-arch.h    |  2 ++
>  arm/include/arm-common/kvm-config-arch.h  |  2 ++
>  arm/kvm-cpu.c                             | 20 ++++++++++++++++++--
>  include/linux/kvm.h                       |  2 ++
>  7 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> index d28ea67..520ea76 100644
> --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
> +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> @@ -13,4 +13,5 @@
>  #define ARM_CPU_ID		0, 0, 0
>  #define ARM_CPU_ID_MPIDR	5
>  
> +#define ARM_VCPU_PTRAUTH_FEATURE	0
>  #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
> index 97c3478..a2546e6 100644
> --- a/arm/aarch64/include/asm/kvm.h
> +++ b/arm/aarch64/include/asm/kvm.h
> @@ -102,6 +102,8 @@ 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_ADDRESS	5 /* CPU uses address pointer authentication */
> +#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* CPU uses generic pointer authentication */
>  
>  struct kvm_vcpu_init {
>  	__u32 target;
> diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
> index 04be43d..0279b13 100644
> --- a/arm/aarch64/include/kvm/kvm-config-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-config-arch.h
> @@ -8,7 +8,11 @@
>  			"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', "enable-ptrauth", &(cfg)->enable_ptrauth,	\
> +			"Enables pointer authentication"),		\
> +	OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth,	\
> +			"Disables pointer 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..fcc2107 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
>  
> +#define ARM_VCPU_PTRAUTH_FEATURE	((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \
> +					| (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC))
>  #endif /* KVM__KVM_CPU_ARCH_H */
> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> index 5734c46..1b4287d 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -10,6 +10,8 @@ struct kvm_config_arch {
>  	bool		aarch32_guest;
>  	bool		has_pmuv3;
>  	u64		kaslr_seed;
> +	bool		enable_ptrauth;
> +	bool		disable_ptrauth;
>  	enum irqchip_type irqchip;
>  	u64		fw_addr;
>  };
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index 7780251..acd1d5f 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -68,6 +68,18 @@ 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);
>  	}
>  
> +	/* Check Pointer Authentication command line arguments. */
> +	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
> +		die("Both enable-ptrauth and disable-ptrauth option cannot be present");

Preferably, print the leading dashes, the same as the user would see
on the command line (e.g., --enable-ptrauth, --disable-ptrauth).

For brevity, we could write something like:

		die("--enable-ptrauth conflicts with --disable-ptrauth");

> +	/*
> +	 * Always enable Pointer Authentication if system supports
> +	 * this extension unless disable-ptrauth option is present.
> +	 */
> +	if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> +	    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) &&
> +	    !kvm->cfg.arch.disable_ptrauth)
> +			vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
> +
>  	/*
>  	 * If the preferred target ioctl is successful then
>  	 * use preferred target else try each and every target type
> @@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>  			die("Unable to find matching target");
>  	}
>  
> -	if (err || target->init(vcpu))
> -		die("Unable to initialise vcpu");
> +	if (err || target->init(vcpu)) {
> +		if (kvm->cfg.arch.enable_ptrauth)
> +			die("Unable to initialise vcpu with pointer authentication feature");

We don't special-case this error message for any other feature yet:
there are a variety of reasons why we might have failed, so suggesting
that the failure is something to do with ptrauth may be misleading to
the user.

If we want to be more informative, we could do something like the
following:

	bool supported;

	supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
		    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC);

	if (kvm->cfg.arch.enable_ptrauth && !supported)
		die("--enable-ptrauth not supported on this host");

	if (supported && !kvm->cfg.arch.disable_ptrauth)
		vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;

	/* ... */

	if (err || target->init(vcpu))
		die("Unable to initialise vcpu");

We don't do this for any other feature today, but since it helps the
user to understand what went wrong it's probably a good idea.

[...]

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest
  2019-04-23 15:44   ` Dave Martin
@ 2019-04-24  5:57     ` Amit Daniel Kachhap
  2019-04-24 13:42       ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-24  5:57 UTC (permalink / raw)
  To: Dave Martin
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

Hi,

On 4/23/19 9:14 PM, Dave Martin wrote:
> On Tue, Apr 23, 2019 at 10:12:34AM +0530, Amit Daniel Kachhap wrote:
>> A per vcpu flag is added to check if pointer authentication is
>> enabled for the vcpu or not. This flag may be enabled according to
>> the necessary user policies and host capabilities.
>>
>> This patch also adds a helper to check the flag.
>>
>> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>> 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: kvmarm@lists.cs.columbia.edu
>> ---
>> Changes since v9:
>>
>> * Added ptrauth cpufeature static check in vcpu_has_ptrauth [Marc Zyngier].
>>
>>   arch/arm64/include/asm/kvm_host.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7a096fd..7ccac42 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -355,10 +355,15 @@ struct kvm_vcpu_arch {
>>   #define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
>>   #define KVM_ARM64_GUEST_HAS_SVE		(1 << 5) /* SVE exposed to guest */
>>   #define KVM_ARM64_VCPU_SVE_FINALIZED	(1 << 6) /* SVE config completed */
>> +#define KVM_ARM64_GUEST_HAS_PTRAUTH	(1 << 7) /* PTRAUTH exposed to guest */
>>   
>>   #define vcpu_has_sve(vcpu) (system_supports_sve() && \
>>   			    ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_SVE))
>>   
>> +#define vcpu_has_ptrauth(vcpu)	((system_supports_address_auth() || \
>> +				  system_supports_generic_auth()) && \
> 
> Come to think of it, should this be
> system_supports_address_auth() _&&_ system_supports_generic_auth()?
I thought about it and kept it this way so that the implementation 
limitation is not introduced in this patch but only in a single place in 
the 3rd patch where all the documentation and reasoning is present on 
doing this way.
> 
> It won't make a functional difference today though, since today
> kvm_vcpu_enable_ptrauth() won't set KVM_ARM64_GUEST_HAS_PTRAUTH without
> system_supports_address_auth() and system_supports_generic_auth() both
> true.
> 
> With || here, we won't have to change this if supporting the two auth
> types independently in the future though.
Yes right.
> 
> Either way, my Reviewed-by stands.
Thanks,
Amit D
> 
> Cheers
> ---Dave
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication
  2019-04-23 15:45   ` Dave Martin
@ 2019-04-24  6:39     ` Amit Daniel Kachhap
  0 siblings, 0 replies; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-24  6:39 UTC (permalink / raw)
  To: Dave Martin
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

Hi,

On 4/23/19 9:15 PM, Dave Martin wrote:
> On Tue, Apr 23, 2019 at 10:12:36AM +0530, Amit Daniel Kachhap wrote:
>> Now that the building blocks of pointer authentication are present, lets
>> add userspace flags KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>> KVM_ARM_VCPU_PTRAUTH_GENERIC. These flags will enable pointer
>> authentication for the KVM guest on a per-vcpu basis through the ioctl
>> KVM_ARM_VCPU_INIT.
>>
>> This features will allow the KVM guest to allow the handling of
>> pointer authentication instructions or to treat them as undefined
>> if not set.
>>
>> Necessary documentations are added to reflect the changes done.
>>
>> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>> 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: kvmarm@lists.cs.columbia.edu
>> ---
>> Changed since v9:
>> * Fixed tab alignment at few places [Dave Martin].
>> * Split the system capability checks [Dave Martin].
>>
>>   Documentation/arm64/pointer-authentication.txt | 22 +++++++++++++++++----
>>   Documentation/virtual/kvm/api.txt              | 10 ++++++++++
>>   arch/arm64/include/asm/kvm_host.h              |  2 +-
>>   arch/arm64/include/uapi/asm/kvm.h              |  2 ++
>>   arch/arm64/kvm/reset.c                         | 27 ++++++++++++++++++++++++++
>>   5 files changed, 58 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
>> index 5baca42..fc71b33 100644
>> --- a/Documentation/arm64/pointer-authentication.txt
>> +++ b/Documentation/arm64/pointer-authentication.txt
>> @@ -87,7 +87,21 @@ used to get and set the keys for a thread.
>>   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 each virtual cpu is
>> +initialised by passing flags KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] and
>> +requesting these two separate cpu features to be enabled. The current KVM
>> +guest implementation works by enabling both features together, so both
>> +these userspace flags are checked before enabling pointer authentication.
>> +The separate userspace flag will allow to have no userspace ABI changes
>> +if support is added in the future to allow these two features to be
>> +enabled independently of one another.
>> +
>> +As Arm Architecture specifies that Pointer Authentication feature is
>> +implemented along with the VHE feature so KVM arm64 ptrauth code relies
>> +on VHE mode to be present.
>> +
>> +Additionally, when these vcpu feature flags are not set then KVM will
>> +filter out the Pointer Authentication system key registers from
>> +KVM_GET/SET_REG_* ioctls and mask those features from cpufeature ID
>> +register. Any attempt to use the Pointer Authentication instructions 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 e410a9f..32afe7f 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2761,6 +2761,16 @@ Possible features:
>>   	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
>>   	  Depends on KVM_CAP_ARM_PMU_V3.
>>   
>> +	- KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>> +	  for arm64 only.
>> +	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
>> +	  must be requested or neither must be requested.
>> +
>> +	- KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>> +	  for arm64 only.
>> +	  Both KVM_ARM_VCPU_PTRAUTH_ADDRESS and KVM_ARM_VCPU_PTRAUTH_GENERIC
>> +	  must be requested or neither must be requested.
>> +
> 
> This looks pretty clear now.
> 
>>   	- KVM_ARM_VCPU_SVE: Enables SVE for the CPU (arm64 only).
>>   	  Depends on KVM_CAP_ARM_SVE.
>>   	  Requires KVM_ARM_VCPU_FINALIZE(KVM_ARM_VCPU_SVE):
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7eebea7..f772ac2 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -49,7 +49,7 @@
>>   
>>   #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>>   
>> -#define KVM_VCPU_MAX_FEATURES 5
>> +#define KVM_VCPU_MAX_FEATURES 7
>>   
>>   #define KVM_REQ_SLEEP \
>>   	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index edd2db8..7b7ac0f 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -104,6 +104,8 @@ 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 */
>>   #define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
>> +#define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
>> +#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
>>   
>>   struct kvm_vcpu_init {
>>   	__u32 target;
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index 3402543..028d0c6 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -221,6 +221,27 @@ static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
>>   		memset(vcpu->arch.sve_state, 0, vcpu_sve_state_size(vcpu));
>>   }
>>   
>> +static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>> +{
>> +	/* Support ptrauth only if the system supports these capabilities. */
>> +	if (!has_vhe())
>> +		return -EINVAL;
>> +
>> +	if (!system_supports_address_auth() ||
>> +	    !system_supports_generic_auth())
>> +		return -EINVAL;
> 
> Since pointer auth implies v8.3 and v8.3 implies v8.1 and v8.1 implies VHE:
> 
> ((system_supports_address_auth() || system_supports_generic_auth()) &&
> !has_vhe()) implies that the hardware is broken or the kernel is buggy.
> 
> So, it probably makes sense to write
> 
> 	if (!system_supports_address_auth() ||
> 	    !system_supports_generic_auth())
> 		return -EINVAL;
> 
> 	if (WARN_ON(!has_vhe()))
> 		return -EINVAL;

Yes agree with you. Now with config VHE restriction set it makes more 
sense that this situation will only occur now in buggy h/w case.
> 
> This is not essential, and doesn't affect the ABI -- so we could apply
> it on top later on.
> 
> [...]
> 
> FWIW:
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com> (for the updates)
Thanks for the review effort.

Thanks,
Amit D
> 
> Cheers
> ---Dave
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-04-23 15:46   ` Dave Martin
@ 2019-04-24  7:02     ` Amit Daniel Kachhap
  2019-04-24 13:41       ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-04-24  7:02 UTC (permalink / raw)
  To: Dave Martin
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

Hi,

On 4/23/19 9:16 PM, Dave Martin wrote:
> On Tue, Apr 23, 2019 at 10:12:38AM +0530, Amit Daniel Kachhap wrote:
>> This patch adds a runtime capabality for KVM tool to enable Arm64 8.3
>> Pointer Authentication in guest kernel. Two vcpu features
>> KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable
>> Pointer Authentication in KVM guest after checking the capability.
>>
>> Command line options --enable-ptrauth and --disable-ptrauth are added
>> to use this feature. However, if those options are not provided then
>> also this feature is enabled if host supports this capability.
>>
>> The macros defined in the headers are not in sync and should be replaced
>> from the upstream.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>>
>> Changes since v9:
>> * Added a error check for both enable-ptrauth and disable-ptrauth
>>    option.
>> * Make the error explicit when enable-ptrauth is provided [Dave Martin].
>>
>>   arm/aarch32/include/kvm/kvm-cpu-arch.h    |  1 +
>>   arm/aarch64/include/asm/kvm.h             |  2 ++
>>   arm/aarch64/include/kvm/kvm-config-arch.h |  6 +++++-
>>   arm/aarch64/include/kvm/kvm-cpu-arch.h    |  2 ++
>>   arm/include/arm-common/kvm-config-arch.h  |  2 ++
>>   arm/kvm-cpu.c                             | 20 ++++++++++++++++++--
>>   include/linux/kvm.h                       |  2 ++
>>   7 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
>> index d28ea67..520ea76 100644
>> --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
>> +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
>> @@ -13,4 +13,5 @@
>>   #define ARM_CPU_ID		0, 0, 0
>>   #define ARM_CPU_ID_MPIDR	5
>>   
>> +#define ARM_VCPU_PTRAUTH_FEATURE	0
>>   #endif /* KVM__KVM_CPU_ARCH_H */
>> diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
>> index 97c3478..a2546e6 100644
>> --- a/arm/aarch64/include/asm/kvm.h
>> +++ b/arm/aarch64/include/asm/kvm.h
>> @@ -102,6 +102,8 @@ 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_ADDRESS	5 /* CPU uses address pointer authentication */
>> +#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* CPU uses generic pointer authentication */
>>   
>>   struct kvm_vcpu_init {
>>   	__u32 target;
>> diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
>> index 04be43d..0279b13 100644
>> --- a/arm/aarch64/include/kvm/kvm-config-arch.h
>> +++ b/arm/aarch64/include/kvm/kvm-config-arch.h
>> @@ -8,7 +8,11 @@
>>   			"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', "enable-ptrauth", &(cfg)->enable_ptrauth,	\
>> +			"Enables pointer authentication"),		\
>> +	OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth,	\
>> +			"Disables pointer 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..fcc2107 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
>>   
>> +#define ARM_VCPU_PTRAUTH_FEATURE	((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \
>> +					| (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC))
>>   #endif /* KVM__KVM_CPU_ARCH_H */
>> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
>> index 5734c46..1b4287d 100644
>> --- a/arm/include/arm-common/kvm-config-arch.h
>> +++ b/arm/include/arm-common/kvm-config-arch.h
>> @@ -10,6 +10,8 @@ struct kvm_config_arch {
>>   	bool		aarch32_guest;
>>   	bool		has_pmuv3;
>>   	u64		kaslr_seed;
>> +	bool		enable_ptrauth;
>> +	bool		disable_ptrauth;
>>   	enum irqchip_type irqchip;
>>   	u64		fw_addr;
>>   };
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index 7780251..acd1d5f 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -68,6 +68,18 @@ 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);
>>   	}
>>   
>> +	/* Check Pointer Authentication command line arguments. */
>> +	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
>> +		die("Both enable-ptrauth and disable-ptrauth option cannot be present");
> 
> Preferably, print the leading dashes, the same as the user would see
> on the command line (e.g., --enable-ptrauth, --disable-ptrauth).
> 
> For brevity, we could write something like:
> 
> 		die("--enable-ptrauth conflicts with --disable-ptrauth");
> 
>> +	/*
>> +	 * Always enable Pointer Authentication if system supports
>> +	 * this extension unless disable-ptrauth option is present.
>> +	 */
>> +	if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>> +	    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) &&
>> +	    !kvm->cfg.arch.disable_ptrauth)
>> +			vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
>> +
>>   	/*
>>   	 * If the preferred target ioctl is successful then
>>   	 * use preferred target else try each and every target type
>> @@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>>   			die("Unable to find matching target");
>>   	}
>>   
>> -	if (err || target->init(vcpu))
>> -		die("Unable to initialise vcpu");
>> +	if (err || target->init(vcpu)) {
>> +		if (kvm->cfg.arch.enable_ptrauth)
>> +			die("Unable to initialise vcpu with pointer authentication feature");
> 
> We don't special-case this error message for any other feature yet:
> there are a variety of reasons why we might have failed, so suggesting
> that the failure is something to do with ptrauth may be misleading to
> the user.
> 
> If we want to be more informative, we could do something like the
> following:
> 
> 	bool supported;
> 
> 	supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> 		    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC);
> 
> 	if (kvm->cfg.arch.enable_ptrauth && !supported)
> 		die("--enable-ptrauth not supported on this host");
> 
> 	if (supported && !kvm->cfg.arch.disable_ptrauth)
> 		vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
> 
> 	/* ... */
> 
> 	if (err || target->init(vcpu))
> 		die("Unable to initialise vcpu");
> 
> We don't do this for any other feature today, but since it helps the
> user to understand what went wrong it's probably a good idea.
Yes this is more clear. As Mark has picked the core guest ptrauth 
patches. I will post this changes as standalone.

Thanks,
Amit Daniel
> 
> [...]
> 
> Cheers
> ---Dave
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23 15:44       ` Dave Martin
@ 2019-04-24 10:29         ` Marc Zyngier
  2019-04-24 13:40           ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Marc Zyngier @ 2019-04-24 10:29 UTC (permalink / raw)
  To: Dave Martin, Amit Daniel Kachhap
  Cc: linux-kernel, Catalin Marinas, Will Deacon, Kristina Martsenko,
	kvmarm, Ramana Radhakrishnan, linux-arm-kernel

On 23/04/2019 16:44, Dave Martin wrote:
> On Tue, Apr 23, 2019 at 03:54:32PM +0530, Amit Daniel Kachhap wrote:
>> Hi Mark,
>>
>> On 4/23/19 3:09 PM, Marc Zyngier wrote:
>>> On Tue, 23 Apr 2019 05:42:35 +0100,
>>> Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>>>>
>>>> From: Mark Rutland <mark.rutland@arm.com>
>>>>
>>>> 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
>>>> in the kernel and present in the 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. However the host key save is
>>>> optimized and implemented inside ptrauth instruction/register access
>>>> trap.
>>>>
>>>> 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). Hence, this patch expects both type of
>>>> authentication to be present in a cpu.
>>>>
>>>> This switch of key is done from guest enter/exit assembly as preparation
>>>> for the upcoming in-kernel pointer authentication support. Hence, these
>>>> key switching routines are not implemented in C code as they may cause
>>>> pointer authentication key signing error in some situations.
>>>>
>>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>>>> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
>>>> , save host key in ptrauth exception trap]
>>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>>> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>>>> Cc: kvmarm@lists.cs.columbia.edu
>>>> ---
>>>> Changes since v9:
>>>>
>>>> * Removed hardcoding of enum values[Mark Zyngier].
>>>> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
>>>> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
>>>> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
>>>>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
>>>> * Added/Modified comments at few places [Marc Zyngier].
>>>>
>>>>  arch/arm/include/asm/kvm_emulate.h   |   2 +
>>>>  arch/arm64/Kconfig                   |   5 +-
>>>>  arch/arm64/include/asm/kvm_emulate.h |  16 ++++++
>>>>  arch/arm64/include/asm/kvm_host.h    |  14 +++++
>>>>  arch/arm64/include/asm/kvm_ptrauth.h | 108 +++++++++++++++++++++++++++++++++++
>>>>  arch/arm64/kernel/asm-offsets.c      |   6 ++
>>>>  arch/arm64/kvm/handle_exit.c         |  36 +++++++++---
>>>>  arch/arm64/kvm/hyp/entry.S           |  15 +++++
>>>>  arch/arm64/kvm/sys_regs.c            |  43 +++++++++++++-
>>>>  virt/kvm/arm/arm.c                   |   2 +
>>>>  10 files changed, 234 insertions(+), 13 deletions(-)
>>>>  create mode 100644 arch/arm64/include/asm/kvm_ptrauth.h
> 
> [...]
> 
>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>>> index 7e34b9e..3cfe2eb 100644
>>>> --- a/arch/arm64/Kconfig
>>>> +++ b/arch/arm64/Kconfig
>>>> @@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
>>>>  	  context-switched along with the process.
>>>>  	  The feature is detected at runtime. If the feature is not present in
>>>> -	  hardware it will not be advertised to userspace nor will it be
>>>> -	  enabled.
>>>> +	  hardware it will not be advertised to userspace/KVM guest nor will it
>>>> +	  be enabled. However, KVM guest also require VHE mode and hence
>>>> +	  CONFIG_ARM64_VHE=y option to use this feature.
>>>
>>> SVE seems to have the exact same requirements, and has
>>>
>>> 	depends on !KVM || ARM64_VHE
>>>
>>> Why don't we have that for PTR_AUTH too?
>> This point came up earlier also and it was suggested by  Dave[1] to leave
>> userspace ptrauth for non-vhe mode as that would bring regression now.
>> [1]:https://lkml.org/lkml/2019/3/27/583
> 
> I see Marc applied this change in
> https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=e19b245fa4c61558536bd34f80845f0c41eab65f0

That's only for me not to forget anything, and it hasn't been folded
into the original patch yet.

> The risk here is that someone has a custom config from an old kernel
> that explicitly turns CONFIG_ARM64_VHE off, and that try to use that
> config with this patch.
> 
> I'm not sure how much we care about that.
> 
> Otherwise, blocking this config so that people don't accidentally rely
> on it seems sensible.

What I'm trying to do is to reduce the amount of valid kernel
configurations that we need to validate independently.

At this stage, I'm tempted to just restrict it as described above, and
maybe relax it if someone shouts at me.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-23  4:42 ` [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers Amit Daniel Kachhap
  2019-04-23  9:39   ` Marc Zyngier
@ 2019-04-24 13:39   ` Dave Martin
  2019-04-24 14:29     ` Marc Zyngier
  1 sibling, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-24 13:39 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: linux-arm-kernel, Marc Zyngier, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Tue, Apr 23, 2019 at 10:12:35AM +0530, Amit Daniel Kachhap wrote:
> From: Mark Rutland <mark.rutland@arm.com>
> 
> 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
> in the kernel and present in the 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. However the host key save is
> optimized and implemented inside ptrauth instruction/register access
> trap.
> 
> 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). Hence, this patch expects both type of
> authentication to be present in a cpu.
> 
> This switch of key is done from guest enter/exit assembly as preparation
> for the upcoming in-kernel pointer authentication support. Hence, these
> key switching routines are not implemented in C code as they may cause
> pointer authentication key signing error in some situations.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
> , save host key in ptrauth exception trap]
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> ---
> Changes since v9:
> 
> * Removed hardcoding of enum values[Mark Zyngier].
> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
> * Added/Modified comments at few places [Marc Zyngier].

[...]

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c

[...]

> @@ -1058,9 +1087,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  					 (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 (!vcpu_has_ptrauth(vcpu)) {
> +			if (val & ptrauth_mask)
> +				kvm_debug("ptrauth unsupported for guests, suppressing\n");
> +			val &= ~ptrauth_mask;
> +		}

Hmmm, didn't spot this before, but this error message no longer makes
sense now that KVM _does_ support pointer auth.

Without vcpu_has_ptrauth(vcpu), we should just silently mask out the
relevant ID fields now (same as for SVE).

The patch below should achieve that.

--8<--

From c6065122c5cccef57108dff990ce8fb43426f88e Mon Sep 17 00:00:00 2001
From: Dave Martin <Dave.Martin@arm.com>
Date: Wed, 24 Apr 2019 14:32:29 +0100
Subject: [PATCH] KVM: arm64: sys_regs: Remove warning about missing pointer
 auth support

KVM does support pointer auth for guests now, so it is
inappropriate (and confusing) to print a warning to dmesg when
userspace explicitly does not ask for pointer auth to be turned on
for a vcpu.

So, just squash the virtual ptrauth ID_AA64ISAR1_EL1 fields when
appropriate and remove the warning.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7f06c2e..f599f5e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1082,16 +1082,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 
 	if (id == SYS_ID_AA64PFR0_EL1 && !vcpu_has_sve(vcpu)) {
 		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 (!vcpu_has_ptrauth(vcpu)) {
-			if (val & ptrauth_mask)
-				kvm_debug("ptrauth unsupported for guests, suppressing\n");
-			val &= ~ptrauth_mask;
-		}
+	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
+		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
+			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
+			 (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
+			 (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
 	}
 
 	return val;
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-24 10:29         ` Marc Zyngier
@ 2019-04-24 13:40           ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2019-04-24 13:40 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Amit Daniel Kachhap, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

On Wed, Apr 24, 2019 at 11:29:37AM +0100, Marc Zyngier wrote:
> On 23/04/2019 16:44, Dave Martin wrote:
> > On Tue, Apr 23, 2019 at 03:54:32PM +0530, Amit Daniel Kachhap wrote:
> >> Hi Mark,
> >>
> >> On 4/23/19 3:09 PM, Marc Zyngier wrote:
> >>> On Tue, 23 Apr 2019 05:42:35 +0100,
> >>> Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:

[...]

> >>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>>> index 7e34b9e..3cfe2eb 100644
> >>>> --- a/arch/arm64/Kconfig
> >>>> +++ b/arch/arm64/Kconfig
> >>>> @@ -1301,8 +1301,9 @@ config ARM64_PTR_AUTH
> >>>>  	  context-switched along with the process.
> >>>>  	  The feature is detected at runtime. If the feature is not present in
> >>>> -	  hardware it will not be advertised to userspace nor will it be
> >>>> -	  enabled.
> >>>> +	  hardware it will not be advertised to userspace/KVM guest nor will it
> >>>> +	  be enabled. However, KVM guest also require VHE mode and hence
> >>>> +	  CONFIG_ARM64_VHE=y option to use this feature.
> >>>
> >>> SVE seems to have the exact same requirements, and has
> >>>
> >>> 	depends on !KVM || ARM64_VHE
> >>>
> >>> Why don't we have that for PTR_AUTH too?
> >> This point came up earlier also and it was suggested by  Dave[1] to leave
> >> userspace ptrauth for non-vhe mode as that would bring regression now.
> >> [1]:https://lkml.org/lkml/2019/3/27/583
> > 
> > I see Marc applied this change in
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=e19b245fa4c61558536bd34f80845f0c41eab65f0
> 
> That's only for me not to forget anything, and it hasn't been folded
> into the original patch yet.

Ah, right, misunderstood.

> > The risk here is that someone has a custom config from an old kernel
> > that explicitly turns CONFIG_ARM64_VHE off, and that try to use that
> > config with this patch.
> > 
> > I'm not sure how much we care about that.
> > 
> > Otherwise, blocking this config so that people don't accidentally rely
> > on it seems sensible.
> 
> What I'm trying to do is to reduce the amount of valid kernel
> configurations that we need to validate independently.
> 
> At this stage, I'm tempted to just restrict it as described above, and
> maybe relax it if someone shouts at me.

Sounds good to me.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-04-24  7:02     ` Amit Daniel Kachhap
@ 2019-04-24 13:41       ` Dave Martin
  2019-05-28 10:11         ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-04-24 13:41 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

On Wed, Apr 24, 2019 at 12:32:22PM +0530, Amit Daniel Kachhap wrote:
> Hi,
> 
> On 4/23/19 9:16 PM, Dave Martin wrote:
> >On Tue, Apr 23, 2019 at 10:12:38AM +0530, Amit Daniel Kachhap wrote:
> >>This patch adds a runtime capabality for KVM tool to enable Arm64 8.3
> >>Pointer Authentication in guest kernel. Two vcpu features
> >>KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable
> >>Pointer Authentication in KVM guest after checking the capability.
> >>
> >>Command line options --enable-ptrauth and --disable-ptrauth are added
> >>to use this feature. However, if those options are not provided then
> >>also this feature is enabled if host supports this capability.
> >>
> >>The macros defined in the headers are not in sync and should be replaced
> >>from the upstream.
> >>
> >>Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> >>---
> >>
> >>Changes since v9:
> >>* Added a error check for both enable-ptrauth and disable-ptrauth
> >>   option.
> >>* Make the error explicit when enable-ptrauth is provided [Dave Martin].
> >>
> >>  arm/aarch32/include/kvm/kvm-cpu-arch.h    |  1 +
> >>  arm/aarch64/include/asm/kvm.h             |  2 ++
> >>  arm/aarch64/include/kvm/kvm-config-arch.h |  6 +++++-
> >>  arm/aarch64/include/kvm/kvm-cpu-arch.h    |  2 ++
> >>  arm/include/arm-common/kvm-config-arch.h  |  2 ++
> >>  arm/kvm-cpu.c                             | 20 ++++++++++++++++++--
> >>  include/linux/kvm.h                       |  2 ++
> >>  7 files changed, 32 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> >>index d28ea67..520ea76 100644
> >>--- a/arm/aarch32/include/kvm/kvm-cpu-arch.h
> >>+++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h
> >>@@ -13,4 +13,5 @@
> >>  #define ARM_CPU_ID		0, 0, 0
> >>  #define ARM_CPU_ID_MPIDR	5
> >>+#define ARM_VCPU_PTRAUTH_FEATURE	0
> >>  #endif /* KVM__KVM_CPU_ARCH_H */
> >>diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
> >>index 97c3478..a2546e6 100644
> >>--- a/arm/aarch64/include/asm/kvm.h
> >>+++ b/arm/aarch64/include/asm/kvm.h
> >>@@ -102,6 +102,8 @@ 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_ADDRESS	5 /* CPU uses address pointer authentication */
> >>+#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* CPU uses generic pointer authentication */
> >>  struct kvm_vcpu_init {
> >>  	__u32 target;
> >>diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h
> >>index 04be43d..0279b13 100644
> >>--- a/arm/aarch64/include/kvm/kvm-config-arch.h
> >>+++ b/arm/aarch64/include/kvm/kvm-config-arch.h
> >>@@ -8,7 +8,11 @@
> >>  			"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', "enable-ptrauth", &(cfg)->enable_ptrauth,	\
> >>+			"Enables pointer authentication"),		\
> >>+	OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth,	\
> >>+			"Disables pointer 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..fcc2107 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
> >>+#define ARM_VCPU_PTRAUTH_FEATURE	((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \
> >>+					| (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC))
> >>  #endif /* KVM__KVM_CPU_ARCH_H */
> >>diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> >>index 5734c46..1b4287d 100644
> >>--- a/arm/include/arm-common/kvm-config-arch.h
> >>+++ b/arm/include/arm-common/kvm-config-arch.h
> >>@@ -10,6 +10,8 @@ struct kvm_config_arch {
> >>  	bool		aarch32_guest;
> >>  	bool		has_pmuv3;
> >>  	u64		kaslr_seed;
> >>+	bool		enable_ptrauth;
> >>+	bool		disable_ptrauth;
> >>  	enum irqchip_type irqchip;
> >>  	u64		fw_addr;
> >>  };
> >>diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> >>index 7780251..acd1d5f 100644
> >>--- a/arm/kvm-cpu.c
> >>+++ b/arm/kvm-cpu.c
> >>@@ -68,6 +68,18 @@ 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);
> >>  	}
> >>+	/* Check Pointer Authentication command line arguments. */
> >>+	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
> >>+		die("Both enable-ptrauth and disable-ptrauth option cannot be present");
> >
> >Preferably, print the leading dashes, the same as the user would see
> >on the command line (e.g., --enable-ptrauth, --disable-ptrauth).
> >
> >For brevity, we could write something like:
> >
> >		die("--enable-ptrauth conflicts with --disable-ptrauth");
> >
> >>+	/*
> >>+	 * Always enable Pointer Authentication if system supports
> >>+	 * this extension unless disable-ptrauth option is present.
> >>+	 */
> >>+	if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> >>+	    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) &&
> >>+	    !kvm->cfg.arch.disable_ptrauth)
> >>+			vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
> >>+
> >>  	/*
> >>  	 * If the preferred target ioctl is successful then
> >>  	 * use preferred target else try each and every target type
> >>@@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
> >>  			die("Unable to find matching target");
> >>  	}
> >>-	if (err || target->init(vcpu))
> >>-		die("Unable to initialise vcpu");
> >>+	if (err || target->init(vcpu)) {
> >>+		if (kvm->cfg.arch.enable_ptrauth)
> >>+			die("Unable to initialise vcpu with pointer authentication feature");
> >
> >We don't special-case this error message for any other feature yet:
> >there are a variety of reasons why we might have failed, so suggesting
> >that the failure is something to do with ptrauth may be misleading to
> >the user.
> >
> >If we want to be more informative, we could do something like the
> >following:
> >
> >	bool supported;
> >
> >	supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> >		    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC);
> >
> >	if (kvm->cfg.arch.enable_ptrauth && !supported)
> >		die("--enable-ptrauth not supported on this host");
> >
> >	if (supported && !kvm->cfg.arch.disable_ptrauth)
> >		vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
> >
> >	/* ... */
> >
> >	if (err || target->init(vcpu))
> >		die("Unable to initialise vcpu");
> >
> >We don't do this for any other feature today, but since it helps the
> >user to understand what went wrong it's probably a good idea.
> Yes this is more clear. As Mark has picked the core guest ptrauth patches. I
> will post this changes as standalone.

Sounds good.  (I also need to do that separately for SVE...)

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest
  2019-04-24  5:57     ` Amit Daniel Kachhap
@ 2019-04-24 13:42       ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2019-04-24 13:42 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

On Wed, Apr 24, 2019 at 11:27:50AM +0530, Amit Daniel Kachhap wrote:
> Hi,
> 
> On 4/23/19 9:14 PM, Dave Martin wrote:
> >On Tue, Apr 23, 2019 at 10:12:34AM +0530, Amit Daniel Kachhap wrote:
> >>A per vcpu flag is added to check if pointer authentication is
> >>enabled for the vcpu or not. This flag may be enabled according to
> >>the necessary user policies and host capabilities.
> >>
> >>This patch also adds a helper to check the flag.
> >>
> >>Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> >>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: kvmarm@lists.cs.columbia.edu
> >>---
> >>Changes since v9:
> >>
> >>* Added ptrauth cpufeature static check in vcpu_has_ptrauth [Marc Zyngier].
> >>
> >>  arch/arm64/include/asm/kvm_host.h | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >>diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>index 7a096fd..7ccac42 100644
> >>--- a/arch/arm64/include/asm/kvm_host.h
> >>+++ b/arch/arm64/include/asm/kvm_host.h
> >>@@ -355,10 +355,15 @@ struct kvm_vcpu_arch {
> >>  #define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
> >>  #define KVM_ARM64_GUEST_HAS_SVE		(1 << 5) /* SVE exposed to guest */
> >>  #define KVM_ARM64_VCPU_SVE_FINALIZED	(1 << 6) /* SVE config completed */
> >>+#define KVM_ARM64_GUEST_HAS_PTRAUTH	(1 << 7) /* PTRAUTH exposed to guest */
> >>  #define vcpu_has_sve(vcpu) (system_supports_sve() && \
> >>  			    ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_SVE))
> >>+#define vcpu_has_ptrauth(vcpu)	((system_supports_address_auth() || \
> >>+				  system_supports_generic_auth()) && \
> >
> >Come to think of it, should this be
> >system_supports_address_auth() _&&_ system_supports_generic_auth()?
> I thought about it and kept it this way so that the implementation
> limitation is not introduced in this patch but only in a single place in the
> 3rd patch where all the documentation and reasoning is present on doing this
> way.

OK, I think that's reasonable.  Just wanted to check that I wasn't
missing some subtle issue here.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-24 13:39   ` Dave Martin
@ 2019-04-24 14:29     ` Marc Zyngier
  2019-04-24 14:30       ` Dave P Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Marc Zyngier @ 2019-04-24 14:29 UTC (permalink / raw)
  To: Dave Martin, Amit Daniel Kachhap
  Cc: linux-arm-kernel, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On 24/04/2019 14:39, Dave Martin wrote:
> On Tue, Apr 23, 2019 at 10:12:35AM +0530, Amit Daniel Kachhap wrote:
>> From: Mark Rutland <mark.rutland@arm.com>
>>
>> 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
>> in the kernel and present in the 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. However the host key save is
>> optimized and implemented inside ptrauth instruction/register access
>> trap.
>>
>> 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). Hence, this patch expects both type of
>> authentication to be present in a cpu.
>>
>> This switch of key is done from guest enter/exit assembly as preparation
>> for the upcoming in-kernel pointer authentication support. Hence, these
>> key switching routines are not implemented in C code as they may cause
>> pointer authentication key signing error in some situations.
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
>> , save host key in ptrauth exception trap]
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>> Cc: kvmarm@lists.cs.columbia.edu
>> ---
>> Changes since v9:
>>
>> * Removed hardcoding of enum values[Mark Zyngier].
>> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
>> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
>> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
>>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
>> * Added/Modified comments at few places [Marc Zyngier].
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> 
> [...]
> 
>> @@ -1058,9 +1087,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>  					 (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 (!vcpu_has_ptrauth(vcpu)) {
>> +			if (val & ptrauth_mask)
>> +				kvm_debug("ptrauth unsupported for guests, suppressing\n");
>> +			val &= ~ptrauth_mask;
>> +		}
> 
> Hmmm, didn't spot this before, but this error message no longer makes
> sense now that KVM _does_ support pointer auth.
> 
> Without vcpu_has_ptrauth(vcpu), we should just silently mask out the
> relevant ID fields now (same as for SVE).
> 
> The patch below should achieve that.
> 
> --8<--
> 
> From c6065122c5cccef57108dff990ce8fb43426f88e Mon Sep 17 00:00:00 2001
> From: Dave Martin <Dave.Martin@arm.com>
> Date: Wed, 24 Apr 2019 14:32:29 +0100
> Subject: [PATCH] KVM: arm64: sys_regs: Remove warning about missing pointer
>  auth support
> 
> KVM does support pointer auth for guests now, so it is
> inappropriate (and confusing) to print a warning to dmesg when
> userspace explicitly does not ask for pointer auth to be turned on
> for a vcpu.
> 
> So, just squash the virtual ptrauth ID_AA64ISAR1_EL1 fields when
> appropriate and remove the warning.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7f06c2e..f599f5e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1082,16 +1082,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  
>  	if (id == SYS_ID_AA64PFR0_EL1 && !vcpu_has_sve(vcpu)) {
>  		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 (!vcpu_has_ptrauth(vcpu)) {
> -			if (val & ptrauth_mask)
> -				kvm_debug("ptrauth unsupported for guests, suppressing\n");
> -			val &= ~ptrauth_mask;
> -		}
> +	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
> +		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
> +			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
> +			 (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
> +			 (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
>  	}
>  
>  	return val;
> 

Since the series isn't in -next yet, I've squashed the above with the
other nits I had in -queue.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers
  2019-04-24 14:29     ` Marc Zyngier
@ 2019-04-24 14:30       ` Dave P Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2019-04-24 14:30 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Amit Kachhap, linux-arm-kernel, Catalin Marinas, Will Deacon,
	Kristina Martsenko, kvmarm, Ramana Radhakrishnan, linux-kernel

On Wed, Apr 24, 2019 at 03:29:26PM +0100, Marc Zyngier wrote:
> On 24/04/2019 14:39, Dave Martin wrote:
> > On Tue, Apr 23, 2019 at 10:12:35AM +0530, Amit Daniel Kachhap wrote:
> >> From: Mark Rutland <mark.rutland@arm.com>
> >>
> >> 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
> >> in the kernel and present in the 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. However the host key save is
> >> optimized and implemented inside ptrauth instruction/register access
> >> trap.
> >>
> >> 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). Hence, this patch expects both type of
> >> authentication to be present in a cpu.
> >>
> >> This switch of key is done from guest enter/exit assembly as preparation
> >> for the upcoming in-kernel pointer authentication support. Hence, these
> >> key switching routines are not implemented in C code as they may cause
> >> pointer authentication key signing error in some situations.
> >>
> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >> [Only VHE, key switch in full assembly, vcpu_has_ptrauth checks
> >> , save host key in ptrauth exception trap]
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> >> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Christoffer Dall <christoffer.dall@arm.com>
> >> Cc: kvmarm@lists.cs.columbia.edu
> >> ---
> >> Changes since v9:
> >>
> >> * Removed hardcoding of enum values[Mark Zyngier].
> >> * Changed kvm_ptrauth_asm.h to kvm_ptrauth.h[Mark Zyngier].
> >> * Removed macro __ptrauth_save_state and applied inline [Marc Zyngier].
> >> * Moved kvm_arm_vcpu_ptrauth_setup_lazy, kvm_arm_vcpu_ptrauth_enable and
> >>   kvm_arm_vcpu_ptrauth_disable from *.c to kvm_emulate.h file [Marc Zyngier].
> >> * Added/Modified comments at few places [Marc Zyngier].

[...]

> >  if (id == SYS_ID_AA64PFR0_EL1 && !vcpu_has_sve(vcpu)) {
> >  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 (!vcpu_has_ptrauth(vcpu)) {
> > -if (val & ptrauth_mask)
> > -kvm_debug("ptrauth unsupported for guests, suppressing\n");
> > -val &= ~ptrauth_mask;
> > -}
> > +} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
> > +val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
> > + (0xfUL << ID_AA64ISAR1_API_SHIFT) |
> > + (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
> > + (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
> >  }
> >
> >  return val;
> >
>
> Since the series isn't in -next yet, I've squashed the above with the
> other nits I had in -queue.

Okey doke.

Thanks
---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] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-04-24 13:41       ` Dave Martin
@ 2019-05-28 10:11         ` Dave Martin
  2019-05-28 12:48           ` Amit Daniel Kachhap
  0 siblings, 1 reply; 26+ messages in thread
From: Dave Martin @ 2019-05-28 10:11 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

On Wed, Apr 24, 2019 at 02:41:21PM +0100, Dave Martin wrote:
> On Wed, Apr 24, 2019 at 12:32:22PM +0530, Amit Daniel Kachhap wrote:
> > Hi,
> > 
> > On 4/23/19 9:16 PM, Dave Martin wrote:

[...]

> > >>diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> > >>index 7780251..acd1d5f 100644
> > >>--- a/arm/kvm-cpu.c
> > >>+++ b/arm/kvm-cpu.c
> > >>@@ -68,6 +68,18 @@ 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);
> > >>  	}
> > >>+	/* Check Pointer Authentication command line arguments. */
> > >>+	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
> > >>+		die("Both enable-ptrauth and disable-ptrauth option cannot be present");
> > >
> > >Preferably, print the leading dashes, the same as the user would see
> > >on the command line (e.g., --enable-ptrauth, --disable-ptrauth).
> > >
> > >For brevity, we could write something like:
> > >
> > >		die("--enable-ptrauth conflicts with --disable-ptrauth");

[...]

> > >>@@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
> > >>  			die("Unable to find matching target");
> > >>  	}
> > >>-	if (err || target->init(vcpu))
> > >>-		die("Unable to initialise vcpu");
> > >>+	if (err || target->init(vcpu)) {
> > >>+		if (kvm->cfg.arch.enable_ptrauth)
> > >>+			die("Unable to initialise vcpu with pointer authentication feature");
> > >
> > >We don't special-case this error message for any other feature yet:
> > >there are a variety of reasons why we might have failed, so suggesting
> > >that the failure is something to do with ptrauth may be misleading to
> > >the user.
> > >
> > >If we want to be more informative, we could do something like the
> > >following:
> > >
> > >	bool supported;
> > >
> > >	supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> > >		    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC);
> > >
> > >	if (kvm->cfg.arch.enable_ptrauth && !supported)
> > >		die("--enable-ptrauth not supported on this host");
> > >
> > >	if (supported && !kvm->cfg.arch.disable_ptrauth)
> > >		vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
> > >
> > >	/* ... */
> > >
> > >	if (err || target->init(vcpu))
> > >		die("Unable to initialise vcpu");
> > >
> > >We don't do this for any other feature today, but since it helps the
> > >user to understand what went wrong it's probably a good idea.
> > Yes this is more clear. As Mark has picked the core guest ptrauth patches. I
> > will post this changes as standalone.
> 
> Sounds good.  (I also need to do that separately for SVE...)

Were you planning to repost this?

Alternatively, I can fix up the diagnostic messages discussed here and
post it together with the SVE support.  I'll do that locally for now,
but let me know what you plan to do.  I'd like to get the SVE support
posted soon so that people can test it.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-05-28 10:11         ` Dave Martin
@ 2019-05-28 12:48           ` Amit Daniel Kachhap
  2019-05-28 13:38             ` Dave Martin
  0 siblings, 1 reply; 26+ messages in thread
From: Amit Daniel Kachhap @ 2019-05-28 12:48 UTC (permalink / raw)
  To: Dave Martin
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

Hi Dave,

On 5/28/19 3:41 PM, Dave Martin wrote:
> On Wed, Apr 24, 2019 at 02:41:21PM +0100, Dave Martin wrote:
>> On Wed, Apr 24, 2019 at 12:32:22PM +0530, Amit Daniel Kachhap wrote:
>>> Hi,
>>>
>>> On 4/23/19 9:16 PM, Dave Martin wrote:
> 
> [...]
> 
>>>>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>>>>> index 7780251..acd1d5f 100644
>>>>> --- a/arm/kvm-cpu.c
>>>>> +++ b/arm/kvm-cpu.c
>>>>> @@ -68,6 +68,18 @@ 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);
>>>>>   	}
>>>>> +	/* Check Pointer Authentication command line arguments. */
>>>>> +	if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth)
>>>>> +		die("Both enable-ptrauth and disable-ptrauth option cannot be present");
>>>>
>>>> Preferably, print the leading dashes, the same as the user would see
>>>> on the command line (e.g., --enable-ptrauth, --disable-ptrauth).
>>>>
>>>> For brevity, we could write something like:
>>>>
>>>> 		die("--enable-ptrauth conflicts with --disable-ptrauth");
> 
> [...]
> 
>>>>> @@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>>>>>   			die("Unable to find matching target");
>>>>>   	}
>>>>> -	if (err || target->init(vcpu))
>>>>> -		die("Unable to initialise vcpu");
>>>>> +	if (err || target->init(vcpu)) {
>>>>> +		if (kvm->cfg.arch.enable_ptrauth)
>>>>> +			die("Unable to initialise vcpu with pointer authentication feature");
>>>>
>>>> We don't special-case this error message for any other feature yet:
>>>> there are a variety of reasons why we might have failed, so suggesting
>>>> that the failure is something to do with ptrauth may be misleading to
>>>> the user.
>>>>
>>>> If we want to be more informative, we could do something like the
>>>> following:
>>>>
>>>> 	bool supported;
>>>>
>>>> 	supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>>> 		    kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC);
>>>>
>>>> 	if (kvm->cfg.arch.enable_ptrauth && !supported)
>>>> 		die("--enable-ptrauth not supported on this host");
>>>>
>>>> 	if (supported && !kvm->cfg.arch.disable_ptrauth)
>>>> 		vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE;
>>>>
>>>> 	/* ... */
>>>>
>>>> 	if (err || target->init(vcpu))
>>>> 		die("Unable to initialise vcpu");
>>>>
>>>> We don't do this for any other feature today, but since it helps the
>>>> user to understand what went wrong it's probably a good idea.
>>> Yes this is more clear. As Mark has picked the core guest ptrauth patches. I
>>> will post this changes as standalone.
>>
>> Sounds good.  (I also need to do that separately for SVE...)
> 
> Were you planning to repost this?
> 
> Alternatively, I can fix up the diagnostic messages discussed here and
> post it together with the SVE support.  I'll do that locally for now,
> but let me know what you plan to do.  I'd like to get the SVE support
> posted soon so that people can test it.
I will clean up the print messages as you suggested and repost it shortly.

Thanks,
Amit Daniel
> 
> Cheers
> ---Dave
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication
  2019-05-28 12:48           ` Amit Daniel Kachhap
@ 2019-05-28 13:38             ` Dave Martin
  0 siblings, 0 replies; 26+ messages in thread
From: Dave Martin @ 2019-05-28 13:38 UTC (permalink / raw)
  To: Amit Daniel Kachhap
  Cc: Marc Zyngier, Catalin Marinas, Will Deacon, linux-kernel,
	Kristina Martsenko, Ramana Radhakrishnan, kvmarm,
	linux-arm-kernel

On Tue, May 28, 2019 at 06:18:16PM +0530, Amit Daniel Kachhap wrote:
> Hi Dave,

[...]

> >Were you planning to repost this?
> >
> >Alternatively, I can fix up the diagnostic messages discussed here and
> >post it together with the SVE support.  I'll do that locally for now,
> >but let me know what you plan to do.  I'd like to get the SVE support
> >posted soon so that people can test it.
> 
> I will clean up the print messages as you suggested and repost it shortly.

OK, thanks.

In the meantime I'll rework the SVE config option stuff on what we
discussed.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2019-05-28 13:38 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23  4:42 [PATCH v10 0/5] Add ARMv8.3 pointer authentication for kvm guest Amit Daniel Kachhap
2019-04-23  4:42 ` [PATCH v10 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest Amit Daniel Kachhap
2019-04-23 15:44   ` Dave Martin
2019-04-24  5:57     ` Amit Daniel Kachhap
2019-04-24 13:42       ` Dave Martin
2019-04-23  4:42 ` [PATCH v10 2/5] KVM: arm/arm64: context-switch ptrauth registers Amit Daniel Kachhap
2019-04-23  9:39   ` Marc Zyngier
2019-04-23 10:24     ` Amit Daniel Kachhap
2019-04-23 15:44       ` Dave Martin
2019-04-24 10:29         ` Marc Zyngier
2019-04-24 13:40           ` Dave Martin
2019-04-24 13:39   ` Dave Martin
2019-04-24 14:29     ` Marc Zyngier
2019-04-24 14:30       ` Dave P Martin
2019-04-23  4:42 ` [PATCH v10 3/5] KVM: arm64: Add userspace flag to enable pointer authentication Amit Daniel Kachhap
2019-04-23 15:45   ` Dave Martin
2019-04-24  6:39     ` Amit Daniel Kachhap
2019-04-23  4:42 ` [PATCH v10 4/5] KVM: arm64: Add capability to advertise ptrauth for guest Amit Daniel Kachhap
2019-04-23 15:45   ` Dave Martin
2019-04-23  4:42 ` [kvmtool PATCH v10 5/5] KVM: arm/arm64: Add a vcpu feature for pointer authentication Amit Daniel Kachhap
2019-04-23 15:46   ` Dave Martin
2019-04-24  7:02     ` Amit Daniel Kachhap
2019-04-24 13:41       ` Dave Martin
2019-05-28 10:11         ` Dave Martin
2019-05-28 12:48           ` Amit Daniel Kachhap
2019-05-28 13:38             ` Dave Martin

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).