Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> When pointer authentication is supported, a guest may wish to use it.
> This patch adds the necessary KVM infrastructure for this to work, with
> a semi-lazy context switch of the pointer auth state.
>
> Pointer authentication feature is only enabled when VHE is built
> into the kernel and present into CPU implementation so only VHE code
~s/into/in the/?
> paths are modified.
>
> When we schedule a vcpu, we disable guest usage of pointer
> authentication instructions and accesses to the keys. While these are
> disabled, we avoid context-switching the keys. When we trap the guest
> trying to use pointer authentication functionality, we change to eagerly
> context-switching the keys, and enable the feature. The next time the
> vcpu is scheduled out/in, we start again.
> Pointer authentication consists of address authentication and generic
> authentication, and CPUs in a system might have varied support for
> either. Where support for either feature is not uniform, it is hidden
> from guests via ID register emulation, as a result of the cpufeature
> framework in the host.
> Unfortunately, address authentication and generic authentication cannot
> be trapped separately, as the architecture provides a single EL2 trap
> covering both. If we wish to expose one without the other, we cannot
> prevent a (badly-written) guest from intermittently using a feature
> which is not uniformly supported (when scheduled on a physical CPU which
> supports the relevant feature). When the guest is scheduled on a
> physical CPU lacking the feature, these attempts will result in an UNDEF
> being taken by the guest.
This won't be fun. Can't KVM check that both are supported on all CPUs to avoid
this? ...
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index dfcfba7..e1bf2a5 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
> cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
> }
>
> +static inline bool kvm_supports_ptrauth(void)
> +{
> + return system_supports_address_auth() && system_supports_generic_auth();
> +}
... oh you do check. Could you cover this in the commit message? (to avoid an
UNDEF being taken by the guest we ... )
cpufeature.h is a strange place to put this, there are no other kvm symbols in
there. But there are users of system_supports_foo() in kvm_host.h.
> diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
> new file mode 100644
> index 0000000..0576c01
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
> + *
> + * Copyright 2018 Arm Limited
> + * Author: Mark Rutland <mark.rutland@xxxxxxx>
> + * Amit Daniel Kachhap <amit.kachhap@xxxxxxx>
> + */
> +#include <linux/compiler.h>
> +#include <linux/kvm_host.h>
> +
> +#include <asm/cpucaps.h>
> +#include <asm/cpufeature.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_hyp.h>
> +#include <asm/pointer_auth.h>
> +
> +static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu *vcpu)
Why __always_inline? Doesn't the compiler decide for 'static' symbols in C files?
> +{
> + return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
> + vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
> +}
> +
> +void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
> + struct kvm_cpu_context *host_ctxt,
> + struct kvm_cpu_context *guest_ctxt)
> +{
> + if (!__ptrauth_is_enabled(vcpu))
> + return;
> +
> + ptrauth_keys_store((struct ptrauth_keys *) &host_ctxt->sys_regs[APIAKEYLO_EL1]);
We can't cast part of an array to a structure like this. What happens if the
compiler inserts padding in struct-ptrauth_keys, or the struct randomization
thing gets hold of it: https://lwn.net/Articles/722293/
If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
of the sys_reg array.
Wouldn't the host keys be available somewhere else? (they must get transfer to
secondary CPUs somehow). Can we skip the save step when switching from the host?
> + ptrauth_keys_switch((struct ptrauth_keys *) &guest_ctxt->sys_regs[APIAKEYLO_EL1]);
> +}
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1f2d237..c798d0c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -439,6 +451,18 @@ static inline bool kvm_arch_requires_vhe(void)
> return false;
> }
>
> +void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
> +void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
> +
> +static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
> +{
> + /* Disable ptrauth and use it in a lazy context via traps */
> + if (has_vhe() && kvm_supports_ptrauth())
> + kvm_arm_vcpu_ptrauth_disable(vcpu);
> +}
(Could you move 'has_vhe()' into kvm_supports_ptrauth()? It fits with testing
the other cpufeatures, and makes this a little more readable when you add
'allowed' to it later.)
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 03b36f1..301d332 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> sysreg_restore_guest_state_vhe(guest_ctxt);
> __debug_switch_to_guest(vcpu);
>
> + __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
> +
> __set_guest_arch_workaround_state(vcpu);
>
> do {
> @@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>
> __set_host_arch_workaround_state(vcpu);
>
> + __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
> +
> sysreg_save_guest_state_vhe(guest_ctxt);
>
> __deactivate_traps(vcpu);
...This makes me nervous...
__guest_enter() is a function that (might) change the keys, then we change them
again here. We can't have any signed return address between these two points. I
don't trust the compiler not to generate any.
~
I had a chat with some friendly compiler folk... because there are two identical
sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
move the common code to a function it then calls. Apparently this is called
'function outlining'.
If the compiler does this, and the guest changes the keys, I think we would fail
the return address check.
Painting the whole thing with no_prauth would solve this, but this code then
becomes a target.
Because the compiler can't anticipate the keys changing, we ought to treat them
the same way we do the callee saved registers, stack-pointer etc, and
save/restore them in the __guest_enter() assembly code.
(we can still keep the save/restore in C, but call it from assembly so we know
nothing new is going on the stack).
Thanks,
James
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
