| 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: <86ttoybbp4.wl-maz@kernel.org>
Date: Mon, 04 Dec 2023 08:42:47 +0000
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
will@...nel.org, catalin.marinas@....com, mark.rutland@....com,
Mark Brown <broonie@...nel.org>,
James Clark <james.clark@....com>,
Rob Herring <robh@...nel.org>,
Suzuki Poulose <suzuki.poulose@....com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
linux-perf-users@...r.kernel.org,
Oliver Upton <oliver.upton@...ux.dev>,
James Morse <james.morse@....com>, kvmarm@...ts.linux.dev
Subject: Re: [PATCH V15 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests
On Fri, 01 Dec 2023 05:39:03 +0000,
Anshuman Khandual <anshuman.khandual@....com> wrote:
>
> Disable the BRBE before we enter the guest, saving the status and enable it
> back once we get out of the guest. This is just to avoid capturing records
> in the guest kernel/userspace, which would be confusing the samples.
Why does it have to be limited to non-VHE? What protects host EL0
records from guest's EL0 execution when the host is VHE?
>
> Cc: Marc Zyngier <maz@...nel.org>
> Cc: Oliver Upton <oliver.upton@...ux.dev>
> Cc: James Morse <james.morse@....com>
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: kvmarm@...ts.linux.dev
> Cc: linux-arm-kernel@...ts.infradead.org
> CC: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
> Changes in V15:
>
> - Dropped runtime BRBE enable for setting DEBUG_STATE_SAVE_BRBE
> - Dropped BRBFCR_EL1 from __debug_save_brbe()/__debug_restore_brbe()
> - Always save the live SYS_BRBCR_EL1 in host context and then check if
> BRBE was enabled before resetting SYS_BRBCR_EL1 for the host
>
> arch/arm64/include/asm/kvm_host.h | 4 ++++
> arch/arm64/kvm/debug.c | 5 +++++
> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 68421c74283a..1faa0430d8dd 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -449,6 +449,8 @@ enum vcpu_sysreg {
> CNTHV_CVAL_EL2,
> PMSCR_EL1, /* Statistical profiling extension */
> TRFCR_EL1, /* Self-hosted trace filters */
> + BRBCR_EL1, /* Branch Record Buffer Control Register */
> + BRBFCR_EL1, /* Branch Record Buffer Function Control Register */
Whose state is this? If this is limited to the host, it has no purpose
in this enum. Once you add guest support, then it will make sense.
>
> NR_SYS_REGS /* Nothing after this line! */
> };
> @@ -753,6 +755,8 @@ struct kvm_vcpu_arch {
> #define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7))
> /* Save trace filter controls */
> #define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8))
> +/* Save BRBE context if active */
> +#define DEBUG_STATE_SAVE_BRBE __vcpu_single_flag(iflags, BIT(9))
>
> /* SVE enabled for host EL0 */
> #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 2ab41b954512..fa46a70a9503 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -354,6 +354,10 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
> !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
> vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> }
> +
> + /* Check if we have BRBE implemented and available at the host */
> + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
> + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
> }
>
> void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
> @@ -361,6 +365,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
> + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
> }
>
> void kvm_etm_set_guest_trfcr(u64 trfcr_guest)
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 6174f710948e..1994fc48b57c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -93,6 +93,33 @@ static void __debug_restore_trace(struct kvm_cpu_context *host_ctxt,
> write_sysreg_s(ctxt_sys_reg(host_ctxt, TRFCR_EL1), SYS_TRFCR_EL1);
> }
>
> +static void __debug_save_brbe(struct kvm_cpu_context *host_ctxt)
> +{
> + ctxt_sys_reg(host_ctxt, BRBCR_EL1) = read_sysreg_s(SYS_BRBCR_EL1);
> +
> + /* Check if the BRBE is enabled */
> + if (!(ctxt_sys_reg(host_ctxt, BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
> + return;
Why save BRBCR_EL1 if there is nothing enabled? It isn't like it can
change behind your back, can it?
> +
> + /*
> + * Prohibit branch record generation while we are in guest.
> + * Since access to BRBCR_EL1 is trapped, the guest can't
> + * modify the filtering set by the host.
> + */
> + write_sysreg_s(0, SYS_BRBCR_EL1);
> + isb();
What is the point of this ISB? We're at EL2, and this only affects
EL1.
> +}
> +
> +static void __debug_restore_brbe(struct kvm_cpu_context *host_ctxt)
> +{
> + if (!ctxt_sys_reg(host_ctxt, BRBCR_EL1))
> + return;
So on one side you're using a flag, and on the other you're using the
*value*. You need some consistency.
> +
> + /* Restore BRBE controls */
> + write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBCR_EL1), SYS_BRBCR_EL1);
> + isb();
Same question.
> +}
> +
> void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
> struct kvm_cpu_context *guest_ctxt)
> {
> @@ -102,6 +129,10 @@ void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>
> if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
> __debug_save_trace(host_ctxt, guest_ctxt);
> +
> + /* Disable BRBE branch records */
> + if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
> + __debug_save_brbe(host_ctxt);
> }
>
> void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -116,6 +147,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
> __debug_restore_spe(host_ctxt);
> if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
> __debug_restore_trace(host_ctxt, guest_ctxt);
> + if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
> + __debug_restore_brbe(host_ctxt);
> }
>
> void __debug_switch_to_host(struct kvm_vcpu *vcpu)
The lifetime of this flag seems bogus, specially when there is nothing
to do, which will always be the arch-majority of the executions.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists