| 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: <a1ea7e74-a90a-4961-9f83-ad04e1d7a573@linux.intel.com>
Date: Tue, 24 Jun 2025 10:43:03 +0800
From: Ethan Zhao <haifeng.zhao@...ux.intel.com>
To: "H. Peter Anvin" <hpa@...or.com>, "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, 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/24 9:53, H. Peter Anvin 写道:
> On June 23, 2025 6:41:07 PM PDT, Ethan Zhao <haifeng.zhao@...ux.intel.com> wrote:
>> 在 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();
> It's split up for the benefit of the stable maintainers.
Undeniably split, as observed. :)
Thanks,
Ethan
>
--
"firm, enduring, strong, and long-lived"
Powered by blists - more mailing lists