| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160201152122.GY1478@cbox>
Date: Mon, 1 Feb 2016 16:21:22 +0100
From: Christoffer Dall <christoffer.dall@...aro.org>
To: Marc Zyngier <marc.zyngier@....com>
Cc: Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu
Subject: Re: [PATCH v2 19/21] arm64: KVM: Move most of the fault decoding to C
On Mon, Jan 25, 2016 at 03:53:53PM +0000, Marc Zyngier wrote:
> The fault decoding process (including computing the IPA in the case
> of a permission fault) would be much better done in C code, as we
> have a reasonable infrastructure to deal with the VHE/non-VHE
> differences.
>
> Let's move the whole thing to C, including the workaround for
> erratum 834220, and just patch the odd ESR_EL2 access remaining
> in hyp-entry.S.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
> ---
> arch/arm64/kernel/asm-offsets.c | 3 --
> arch/arm64/kvm/hyp/hyp-entry.S | 69 +++--------------------------------------
> arch/arm64/kvm/hyp/switch.c | 54 ++++++++++++++++++++++++++++++++
> 3 files changed, 59 insertions(+), 67 deletions(-)
>
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index fffa4ac6..b0ab4e9 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -110,9 +110,6 @@ int main(void)
> DEFINE(CPU_USER_PT_REGS, offsetof(struct kvm_regs, regs));
> DEFINE(CPU_FP_REGS, offsetof(struct kvm_regs, fp_regs));
> DEFINE(VCPU_FPEXC32_EL2, offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2]));
> - DEFINE(VCPU_ESR_EL2, offsetof(struct kvm_vcpu, arch.fault.esr_el2));
> - DEFINE(VCPU_FAR_EL2, offsetof(struct kvm_vcpu, arch.fault.far_el2));
> - DEFINE(VCPU_HPFAR_EL2, offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
> DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context));
> #endif
> #ifdef CONFIG_CPU_PM
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 9e0683f..213de52 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -19,7 +19,6 @@
>
> #include <asm/alternative.h>
> #include <asm/assembler.h>
> -#include <asm/asm-offsets.h>
> #include <asm/cpufeature.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> @@ -67,7 +66,11 @@ ENDPROC(__vhe_hyp_call)
> el1_sync: // Guest trapped into EL2
> save_x0_to_x3
>
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> mrs x1, esr_el2
> +alternative_else
> + mrs x1, esr_el1
> +alternative_endif
I suppose this is not technically part of what the patch description
says it does, but ok...
> lsr x2, x1, #ESR_ELx_EC_SHIFT
>
> cmp x2, #ESR_ELx_EC_HVC64
> @@ -103,72 +106,10 @@ el1_trap:
> cmp x2, #ESR_ELx_EC_FP_ASIMD
> b.eq __fpsimd_guest_restore
>
> - cmp x2, #ESR_ELx_EC_DABT_LOW
> - mov x0, #ESR_ELx_EC_IABT_LOW
> - ccmp x2, x0, #4, ne
> - b.ne 1f // Not an abort we care about
> -
> - /* This is an abort. Check for permission fault */
> -alternative_if_not ARM64_WORKAROUND_834220
> - and x2, x1, #ESR_ELx_FSC_TYPE
> - cmp x2, #FSC_PERM
> - b.ne 1f // Not a permission fault
> -alternative_else
> - nop // Use the permission fault path to
> - nop // check for a valid S1 translation,
> - nop // regardless of the ESR value.
> -alternative_endif
> -
> - /*
> - * Check for Stage-1 page table walk, which is guaranteed
> - * to give a valid HPFAR_EL2.
> - */
> - tbnz x1, #7, 1f // S1PTW is set
> -
> - /* Preserve PAR_EL1 */
> - mrs x3, par_el1
> - stp x3, xzr, [sp, #-16]!
> -
> - /*
> - * Permission fault, HPFAR_EL2 is invalid.
> - * Resolve the IPA the hard way using the guest VA.
> - * Stage-1 translation already validated the memory access rights.
> - * As such, we can use the EL1 translation regime, and don't have
> - * to distinguish between EL0 and EL1 access.
> - */
> - mrs x2, far_el2
> - at s1e1r, x2
> - isb
> -
> - /* Read result */
> - mrs x3, par_el1
> - ldp x0, xzr, [sp], #16 // Restore PAR_EL1 from the stack
> - msr par_el1, x0
> - tbnz x3, #0, 3f // Bail out if we failed the translation
> - ubfx x3, x3, #12, #36 // Extract IPA
> - lsl x3, x3, #4 // and present it like HPFAR
> - b 2f
> -
> -1: mrs x3, hpfar_el2
> - mrs x2, far_el2
> -
> -2: mrs x0, tpidr_el2
> - str w1, [x0, #VCPU_ESR_EL2]
> - str x2, [x0, #VCPU_FAR_EL2]
> - str x3, [x0, #VCPU_HPFAR_EL2]
> -
> + mrs x0, tpidr_el2
> mov x1, #ARM_EXCEPTION_TRAP
> b __guest_exit
>
> - /*
> - * Translation failed. Just return to the guest and
> - * let it fault again. Another CPU is probably playing
> - * behind our back.
> - */
> -3: restore_x0_to_x3
> -
> - eret
> -
> el1_irq:
> save_x0_to_x3
> mrs x0, tpidr_el2
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 0cadb7f..df2cce9 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -15,6 +15,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <linux/types.h>
> #include <asm/kvm_asm.h>
>
> #include "hyp.h"
> @@ -150,6 +151,55 @@ static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu)
> __vgic_call_restore_state()(vcpu);
> }
>
> +static hyp_alternate_value(__check_arm_834220,
> + false, true,
> + ARM64_WORKAROUND_834220);
> +
> +static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
> +{
> + u64 esr = read_sysreg_el2(esr);
> + u8 ec = esr >> ESR_ELx_EC_SHIFT;
> + u64 hpfar, far;
> +
> + vcpu->arch.fault.esr_el2 = esr;
> +
> + if (ec != ESR_ELx_EC_DABT_LOW && ec != ESR_ELx_EC_IABT_LOW)
> + return true;
> +
> + far = read_sysreg_el2(far);
> +
> + if (!(esr & ESR_ELx_S1PTW) &&
> + (__check_arm_834220() || (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) {
this is really hard to read. How do you feel about putting the below
block into its own function and changing to something like this:
/*
* The HPFAR can be invalid if the stage 2 fault did not happen during a
* stage 1 page table walk (the ESR_EL2.S1PTW bit is clear) and one of
* the two following cases are true:
* 1. The fault was due to a permission fault
* 2. The processor carries errata 834220
*
* Therefore, for all non S1PTW faults where we either have a permission
* fault or the errata workaround is enabled, we resolve the IPA using
* the AT instruction.
*/
if (!(esr & ESR_ELx_S1PTW) &&
(__check_arm_834220() || (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) {
if (!__translate_far_to_ipa(&hpfar))
return false; /* Translation failed, back to guest */
} else {
hpfar = read_sysreg(hpfar_el2);
}
not sure if it helps that much, perhaps it's just complicated by nature.
> + u64 par, tmp;
> +
> + /*
> + * Permission fault, HPFAR_EL2 is invalid. Resolve the
> + * IPA the hard way using the guest VA.
> + * Stage-1 translation already validated the memory
> + * access rights. As such, we can use the EL1
> + * translation regime, and don't have to distinguish
> + * between EL0 and EL1 access.
> + */
> + par = read_sysreg(par_el1);
in any cas I think we also need the comment about preserving par_el1
here, which is only something we do because we may return early, IIUC.
> + asm volatile("at s1e1r, %0" : : "r" (far));
> + isb();
> +
> + tmp = read_sysreg(par_el1);
> + write_sysreg(par, par_el1);
> +
> + if (unlikely(tmp & 1))
> + return false; /* Translation failed, back to guest */
> +
nit: add comment /* Convert PAR to HPFAR format */
> + hpfar = ((tmp >> 12) & ((1UL << 36) - 1)) << 4;
> + } else {
> + hpfar = read_sysreg(hpfar_el2);
> + }
> +
> + vcpu->arch.fault.far_el2 = far;
> + vcpu->arch.fault.hpfar_el2 = hpfar;
> + return true;
> +}
> +
> static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
> {
> struct kvm_cpu_context *host_ctxt;
> @@ -181,9 +231,13 @@ static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
> __debug_restore_state(vcpu, kern_hyp_va(vcpu->arch.debug_ptr), guest_ctxt);
>
> /* Jump in the fire! */
> +again:
> exit_code = __guest_enter(vcpu, host_ctxt);
> /* And we're baaack! */
>
> + if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
> + goto again;
> +
> fp_enabled = __fpsimd_enabled();
>
> __sysreg_save_guest_state(guest_ctxt);
> --
> 2.1.4
>
The good news are that I couldn't find any bugs in the code.
-Christoffer
Powered by blists - more mailing lists