| 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: <c526eb25-571e-427a-93e9-3afdaa6ca413@linux.intel.com>
Date: Tue, 24 Jun 2025 09:41:07 +0800
From: Ethan Zhao <haifeng.zhao@...ux.intel.com>
To: "Xin Li (Intel)" <xin@...or.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, stable@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
seanjc@...gle.com, pbonzini@...hat.com, peterz@...radead.org,
sohil.mehta@...el.com, brgerst@...il.com, tony.luck@...el.com,
fenghuay@...dia.com
Subject: Re: [PATCH v4 2/2] x86/traps: Initialize DR7 by writing its
architectural reset value
在 2025/6/21 7:15, Xin Li (Intel) 写道:
> Initialize DR7 by writing its architectural reset value to always set
> bit 10, which is reserved to '1', when "clearing" DR7 so as not to
> trigger unanticipated behavior if said bit is ever unreserved, e.g. as
> a feature enabling flag with inverted polarity.
>
> Tested-by: Sohil Mehta <sohil.mehta@...el.com>
> Reviewed-by: H. Peter Anvin (Intel) <hpa@...or.com>
> Reviewed-by: Sohil Mehta <sohil.mehta@...el.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Acked-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> Cc: stable@...r.kernel.org
> ---
>
> Change in v4:
> *) Cc stable for backporting, just in case bit 10 of DR7 has become
> unreserved on new hardware, even though clearing it doesn't
> currently cause any real issues (Dave Hansen).
>
> Changes in v3:
> *) Reword the changelog using Sean's description.
> *) Explain the definition of DR7_FIXED_1 (Sohil).
> *) Collect TB, RB, AB (PeterZ, Sohil and Sean).
>
> Changes in v2:
> *) Use debug register index 7 rather than DR_CONTROL (PeterZ and Sean).
> *) Use DR7_FIXED_1 as the architectural reset value of DR7 (Sean).
> ---
> arch/x86/include/asm/debugreg.h | 19 +++++++++++++++----
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kernel/cpu/common.c | 2 +-
> arch/x86/kernel/kgdb.c | 2 +-
> arch/x86/kernel/process_32.c | 2 +-
> arch/x86/kernel/process_64.c | 2 +-
> arch/x86/kvm/x86.c | 4 ++--
> 7 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index 363110e6b2e3..a2c1f2d24b64 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -9,6 +9,14 @@
> #include <asm/cpufeature.h>
> #include <asm/msr.h>
>
> +/*
> + * Define bits that are always set to 1 in DR7, only bit 10 is
> + * architecturally reserved to '1'.
> + *
> + * This is also the init/reset value for DR7.
> + */
> +#define DR7_FIXED_1 0x00000400
> +
> DECLARE_PER_CPU(unsigned long, cpu_dr7);
>
> #ifndef CONFIG_PARAVIRT_XXL
> @@ -100,8 +108,8 @@ static __always_inline void native_set_debugreg(int regno, unsigned long value)
>
> static inline void hw_breakpoint_disable(void)
> {
> - /* Zero the control register for HW Breakpoint */
> - set_debugreg(0UL, 7);
> + /* Reset the control register for HW Breakpoint */
> + set_debugreg(DR7_FIXED_1, 7);
Given you have it be adhere to SDM about the DR7 reversed bits setting,
then no reason to leave patch[1/2] to set_debugreg(0, 7) alone.
did I miss something here ?
Thanks,
Ethan
>
> /* Zero-out the individual HW breakpoint address registers */
> set_debugreg(0UL, 0);
> @@ -125,9 +133,12 @@ static __always_inline unsigned long local_db_save(void)
> return 0;
>
> get_debugreg(dr7, 7);
> - dr7 &= ~0x400; /* architecturally set bit */
> +
> + /* Architecturally set bit */
> + dr7 &= ~DR7_FIXED_1;
> if (dr7)
> - set_debugreg(0, 7);
> + set_debugreg(DR7_FIXED_1, 7);
> +
> /*
> * Ensure the compiler doesn't lower the above statements into
> * the critical section; disabling breakpoints late would not
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b4a391929cdb..639d9bcee842 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -31,6 +31,7 @@
>
> #include <asm/apic.h>
> #include <asm/pvclock-abi.h>
> +#include <asm/debugreg.h>
> #include <asm/desc.h>
> #include <asm/mtrr.h>
> #include <asm/msr-index.h>
> @@ -249,7 +250,6 @@ enum x86_intercept_stage;
> #define DR7_BP_EN_MASK 0x000000ff
> #define DR7_GE (1 << 9)
> #define DR7_GD (1 << 13)
> -#define DR7_FIXED_1 0x00000400
> #define DR7_VOLATILE 0xffff2bff
>
> #define KVM_GUESTDBG_VALID_MASK \
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 0f6c280a94f0..27125e009847 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2246,7 +2246,7 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
> static void initialize_debug_regs(void)
> {
> /* Control register first -- to make sure everything is disabled. */
> - set_debugreg(0, 7);
> + set_debugreg(DR7_FIXED_1, 7);
> set_debugreg(DR6_RESERVED, 6);
> /* dr5 and dr4 don't exist */
> set_debugreg(0, 3);
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index 102641fd2172..8b1a9733d13e 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -385,7 +385,7 @@ static void kgdb_disable_hw_debug(struct pt_regs *regs)
> struct perf_event *bp;
>
> /* Disable hardware debugging while we are in kgdb: */
> - set_debugreg(0UL, 7);
> + set_debugreg(DR7_FIXED_1, 7);
> for (i = 0; i < HBP_NUM; i++) {
> if (!breakinfo[i].enabled)
> continue;
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index a10e180cbf23..3ef15c2f152f 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -93,7 +93,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
>
> /* Only print out debug registers if they are in their non-default state. */
> if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
> - (d6 == DR6_RESERVED) && (d7 == 0x400))
> + (d6 == DR6_RESERVED) && (d7 == DR7_FIXED_1))
> return;
>
> printk("%sDR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 8d6cf25127aa..b972bf72fb8b 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -133,7 +133,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
>
> /* Only print out debug registers if they are in their non-default state. */
> if (!((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
> - (d6 == DR6_RESERVED) && (d7 == 0x400))) {
> + (d6 == DR6_RESERVED) && (d7 == DR7_FIXED_1))) {
> printk("%sDR0: %016lx DR1: %016lx DR2: %016lx\n",
> log_lvl, d0, d1, d2);
> printk("%sDR3: %016lx DR6: %016lx DR7: %016lx\n",
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b58a74c1722d..a9d992d5652f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11035,7 +11035,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>
> if (unlikely(vcpu->arch.switch_db_regs &&
> !(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH))) {
> - set_debugreg(0, 7);
> + set_debugreg(DR7_FIXED_1, 7);
> set_debugreg(vcpu->arch.eff_db[0], 0);
> set_debugreg(vcpu->arch.eff_db[1], 1);
> set_debugreg(vcpu->arch.eff_db[2], 2);
> @@ -11044,7 +11044,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
> kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
> } else if (unlikely(hw_breakpoint_active())) {
> - set_debugreg(0, 7);
> + set_debugreg(DR7_FIXED_1, 7);
> }
>
> vcpu->arch.host_debugctl = get_debugctlmsr();
--
"firm, enduring, strong, and long-lived"
Powered by blists - more mailing lists