lkml.org 
[lkml]   [2019]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6 3/6] arm64/kvm: context-switch ptrauth registers
    From
    Date
    Hi,

    On 2/21/19 9:23 PM, Dave Martin wrote:
    > On Tue, Feb 19, 2019 at 02:54:28PM +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 into CPU implementation so only VHE code
    >> paths are modified.
    >>
    >> When we schedule a vcpu, we disable guest usage of pointer
    >> authentication instructions and accesses to the keys. While these are
    >> disabled, we avoid context-switching the keys. When we trap the guest
    >> trying to use pointer authentication functionality, we change to eagerly
    >> context-switching the keys, and enable the feature. The next time the
    >> vcpu is scheduled out/in, we start again. However the host key registers
    >> are saved in vcpu load stage as they remain constant for each vcpu
    >> schedule.
    >>
    >> 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.
    >>
    >> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
    >> [Only VHE, key switch from from assembly, kvm_supports_ptrauth
    >> checks, save host key in vcpu_load]
    >> 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
    >> ---
    >> arch/arm/include/asm/kvm_host.h | 1 +
    >> arch/arm64/include/asm/kvm_host.h | 23 +++++++++
    >> arch/arm64/include/asm/kvm_hyp.h | 7 +++
    >> arch/arm64/kernel/traps.c | 1 +
    >> arch/arm64/kvm/handle_exit.c | 21 +++++---
    >> arch/arm64/kvm/hyp/Makefile | 1 +
    >> arch/arm64/kvm/hyp/entry.S | 17 +++++++
    >> arch/arm64/kvm/hyp/ptrauth-sr.c | 101 ++++++++++++++++++++++++++++++++++++++
    >> arch/arm64/kvm/sys_regs.c | 37 +++++++++++++-
    >> virt/kvm/arm/arm.c | 2 +
    >> 10 files changed, 201 insertions(+), 10 deletions(-)
    >> create mode 100644 arch/arm64/kvm/hyp/ptrauth-sr.c
    >
    > [...]
    >
    >> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
    >> new file mode 100644
    >> index 0000000..528ee6e
    >> --- /dev/null
    >> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
    >> @@ -0,0 +1,101 @@
    >> +// SPDX-License-Identifier: GPL-2.0
    >> +/*
    >> + * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
    >> + *
    >> + * Copyright 2018 Arm Limited
    >> + * Author: Mark Rutland <mark.rutland@arm.com>
    >> + * Amit Daniel Kachhap <amit.kachhap@arm.com>
    >> + */
    >> +#include <linux/compiler.h>
    >> +#include <linux/kvm_host.h>
    >> +
    >> +#include <asm/cpucaps.h>
    >> +#include <asm/cpufeature.h>
    >> +#include <asm/kvm_asm.h>
    >> +#include <asm/kvm_hyp.h>
    >> +#include <asm/pointer_auth.h>
    >> +
    >> +static __always_inline bool __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
    >> +{
    >> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
    >> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
    >> +}
    >> +
    >> +#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); \
    >> +})
    >> +
    >> +static __always_inline void __ptrauth_save_state(struct kvm_cpu_context *ctxt)
    >
    > Why __always_inline?
    >
    >> +{
    >> + __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);
    >> +}
    >> +
    >> +#define __ptrauth_restore_key(regs, key) \
    >> +({ \
    >> + write_sysreg_s(regs[key ## KEYLO_EL1], SYS_ ## key ## KEYLO_EL1); \
    >> + write_sysreg_s(regs[key ## KEYHI_EL1], SYS_ ## key ## KEYHI_EL1); \
    >> +})
    >> +
    >> +static __always_inline void __ptrauth_restore_state(struct kvm_cpu_context *ctxt)
    >
    > Same here. I would hope these just need to be marked with the correct
    > function attribute to disable ptrauth by the compiler. I don't see why
    > it makes a difference whether it's inline or not.
    >
    > If the compiler semantics are not sufficiently clear, make it a macro.
    ok.
    >
    > (Bikeshedding here, so it you feel this has already been discussed to
    > death I'm happy for this to stay as-is.)
    >
    >> +{
    >> + __ptrauth_restore_key(ctxt->sys_regs, APIA);
    >> + __ptrauth_restore_key(ctxt->sys_regs, APIB);
    >> + __ptrauth_restore_key(ctxt->sys_regs, APDA);
    >> + __ptrauth_restore_key(ctxt->sys_regs, APDB);
    >> + __ptrauth_restore_key(ctxt->sys_regs, APGA);
    >> +}
    >> +
    >> +/**
    >> + * This function changes the key so assign Pointer Authentication safe
    >> + * GCC attribute if protected by it.
    >> + */
    >
    > (I'd have preferred to keep __noptrauth here and define it do nothing for
    > now. But I'll defer to others on that, since this has already been
    > discussed...)

    ok __noptrauth annotation will make it clear. I will add it for all C
    error prone functions in the next iteration.
    >
    >> +void __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
    >> + struct kvm_cpu_context *host_ctxt,
    >> + struct kvm_cpu_context *guest_ctxt)
    >> +{
    >> + if (!__ptrauth_is_enabled(vcpu))
    >> + return;
    >> +
    >> + __ptrauth_restore_state(guest_ctxt);
    >> +}
    >> +
    >> +/**
    >> + * This function changes the key so assign Pointer Authentication safe
    >> + * GCC attribute if protected by it.
    >> + */
    >> +void __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
    >> + struct kvm_cpu_context *guest_ctxt,
    >> + struct kvm_cpu_context *host_ctxt)
    >> +{
    >> + if (!__ptrauth_is_enabled(vcpu))
    >> + return;
    >> +
    >> + __ptrauth_save_state(guest_ctxt);
    >> + __ptrauth_restore_state(host_ctxt);
    >> +}
    >> +
    >> +/**
    >> + * kvm_arm_vcpu_ptrauth_reset - resets ptrauth for vcpu schedule
    >> + *
    >> + * @vcpu: The VCPU pointer
    >> + *
    >> + * This function may be used to disable ptrauth and use it in a lazy context
    >> + * via traps. However host key registers are saved here as they dont change
    >> + * during host/guest switch.
    >> + */
    >> +void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
    >
    > I feel this is not a good name. It sounds too much like it resets the
    > registers as part of vcpu reset, whereas really it's doing something
    > completely different.
    >
    > (Do you reset the regs anywhere btw? I may have missed it...)
    No there is not reset of registers. May be name like
    kvm_arm_vcpu_ptrauth_setup_lazy would be better.
    >
    >> +{
    >> + struct kvm_cpu_context *host_ctxt;
    >> +
    >> + if (kvm_supports_ptrauth()) {
    >> + kvm_arm_vcpu_ptrauth_disable(vcpu);
    >> + host_ctxt = vcpu->arch.host_cpu_context;
    >> + __ptrauth_save_state(host_ctxt);
    >> + }
    >> +}
    >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
    >> index a6c9381..12529df 100644
    >> --- a/arch/arm64/kvm/sys_regs.c
    >> +++ b/arch/arm64/kvm/sys_regs.c
    >> @@ -986,6 +986,32 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
    >> { SYS_DESC(SYS_PMEVTYPERn_EL0(n)), \
    >> access_pmu_evtyper, reset_unknown, (PMEVTYPER0_EL0 + n), }
    >>
    >> +
    >> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
    >> +{
    >> + vcpu->arch.ctxt.hcr_el2 |= (HCR_API | HCR_APK);
    >
    > Pedantic nit: surplus ().
    >
    > (Although opinions differ, and keeping them looks more symmetric with
    > kvm_arm_vcpu_ptrauth_disable() -- either way, the code can stay as-is if
    > you prefer.)
    ok.
    >
    >> +}
    >> +
    >> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu)
    >> +{
    >> + vcpu->arch.ctxt.hcr_el2 &= ~(HCR_API | HCR_APK);
    >> +}
    >> +
    >> +static bool trap_ptrauth(struct kvm_vcpu *vcpu,
    >> + struct sys_reg_params *p,
    >> + const struct sys_reg_desc *rd)
    >> +{
    >> + kvm_arm_vcpu_ptrauth_trap(vcpu);
    >> + return false;
    >
    > Can we ever get here? Won't PAC traps always be handled via
    > handle_exit()?
    >
    > Or can we also take sysreg access traps when the guest tries to access
    > the ptrauth key registers?

    When Guest kernel forks thread then the key registers are accessed to
    fill them with ptrauth keys and hcr_el2(APK bit) is not appropriately
    set at that moment. This causes trap to EL2 and the above function is
    invoked.
    >
    > (I'm now wondering how this works for SVE.)
    Not sure. Need to check.
    >
    >> +}
    >> +
    >> +#define __PTRAUTH_KEY(k) \
    >> + { SYS_DESC(SYS_## k), trap_ptrauth, reset_unknown, k }
    >> +
    >> +#define PTRAUTH_KEY(k) \
    >> + __PTRAUTH_KEY(k ## KEYLO_EL1), \
    >> + __PTRAUTH_KEY(k ## KEYHI_EL1)
    >> +
    >> static bool access_cntp_tval(struct kvm_vcpu *vcpu,
    >> struct sys_reg_params *p,
    >> const struct sys_reg_desc *r)
    >> @@ -1045,9 +1071,10 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
    >> (0xfUL << ID_AA64ISAR1_API_SHIFT) |
    >> (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
    >> (0xfUL << ID_AA64ISAR1_GPI_SHIFT);
    >> - if (val & ptrauth_mask)
    >> + if (!kvm_supports_ptrauth()) {
    >
    > Don't we now always print this when ptrauth is not supported?
    >
    > Previously we only printed a message in the interesting case, i.e.,
    > where the host supports ptrauch but we cannot offer it to the guest.
    Yes agreed. I will add proper checks here to skip prints for non ptrauth
    hosts.

    Thanks,
    Amit D
    >
    >> kvm_debug("ptrauth unsupported for guests, suppressing\n");
    >> - val &= ~ptrauth_mask;
    >> + val &= ~ptrauth_mask;
    >> + }
    >> } else if (id == SYS_ID_AA64MMFR1_EL1) {
    >> if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
    >> kvm_debug("LORegions unsupported for guests, suppressing\n");
    >> @@ -1316,6 +1343,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 2032a66..d7e003f 100644
    >> --- a/virt/kvm/arm/arm.c
    >> +++ b/virt/kvm/arm/arm.c
    >> @@ -388,6 +388,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
    >> vcpu_clear_wfe_traps(vcpu);
    >> else
    >> vcpu_set_wfe_traps(vcpu);
    >> +
    >> + kvm_arm_vcpu_ptrauth_reset(vcpu);
    >> }
    >>
    >> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
    >> --
    >> 2.7.4
    >>
    >> _______________________________________________
    >> kvmarm mailing list
    >> kvmarm@lists.cs.columbia.edu
    >> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

    \
     
     \ /
      Last update: 2019-03-01 10:36    [W:4.141 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site