| 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: <5bd4df17-4df2-4423-a504-56c56b9f43f8@linux.intel.com>
Date: Tue, 24 Jun 2025 09:06:52 +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 1/2] x86/traps: Initialize DR6 by writing its
architectural reset value
在 2025/6/23 16:14, H. Peter Anvin 写道:
> On June 22, 2025 11:49:11 PM PDT, Ethan Zhao <haifeng.zhao@...ux.intel.com> wrote:
>> 在 2025/6/21 7:15, Xin Li (Intel) 写道:
>>> Initialize DR6 by writing its architectural reset value to avoid
>>> incorrectly zeroing DR6 to clear DR6.BLD at boot time, which leads
>>> to a false bus lock detected warning.
>>>
>>> The Intel SDM says:
>>>
>>> 1) Certain debug exceptions may clear bits 0-3 of DR6.
>>>
>>> 2) BLD induced #DB clears DR6.BLD and any other debug exception
>>> doesn't modify DR6.BLD.
>>>
>>> 3) RTM induced #DB clears DR6.RTM and any other debug exception
>>> sets DR6.RTM.
>>>
>>> To avoid confusion in identifying debug exceptions, debug handlers
>>> should set DR6.BLD and DR6.RTM, and clear other DR6 bits before
>>> returning.
>>>
>>> The DR6 architectural reset value 0xFFFF0FF0, already defined as
>>> macro DR6_RESERVED, satisfies these requirements, so just use it to
>>> reinitialize DR6 whenever needed.
>>>
>>> Since clear_all_debug_regs() no longer zeros all debug registers,
>>> rename it to initialize_debug_regs() to better reflect its current
>>> behavior.
>>>
>>> Since debug_read_clear_dr6() no longer clears DR6, rename it to
>>> debug_read_reset_dr6() to better reflect its current behavior.
>>>
>>> Reported-by: Sohil Mehta <sohil.mehta@...el.com>
>>> Link: https://lore.kernel.org/lkml/06e68373-a92b-472e-8fd9-ba548119770c@intel.com/
>>> Fixes: ebb1064e7c2e9 ("x86/traps: Handle #DB for bus lock")
>>> Suggested-by: H. Peter Anvin (Intel) <hpa@...or.com>
>>> 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>
>>> Signed-off-by: Xin Li (Intel) <xin@...or.com>
>>> Cc: stable@...r.kernel.org
>>> ---
>>>
>>> Changes in v3:
>>> *) Polish initialize_debug_regs() (PeterZ).
>>> *) Rewrite the comment for DR6_RESERVED definition (Sohil and Sean).
>>> *) Collect TB, RB, AB (PeterZ and Sohil).
>>>
>>> Changes in v2:
>>> *) Use debug register index 6 rather than DR_STATUS (PeterZ and Sean).
>>> *) Move this patch the first of the patch set to ease backporting.
>>> ---
>>> arch/x86/include/uapi/asm/debugreg.h | 21 ++++++++++++++++-
>>> arch/x86/kernel/cpu/common.c | 24 ++++++++------------
>>> arch/x86/kernel/traps.c | 34 +++++++++++++++++-----------
>>> 3 files changed, 51 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
>>> index 0007ba077c0c..41da492dfb01 100644
>>> --- a/arch/x86/include/uapi/asm/debugreg.h
>>> +++ b/arch/x86/include/uapi/asm/debugreg.h
>>> @@ -15,7 +15,26 @@
>>> which debugging register was responsible for the trap. The other bits
>>> are either reserved or not of interest to us. */
>>> -/* Define reserved bits in DR6 which are always set to 1 */
>>> +/*
>>> + * Define bits in DR6 which are set to 1 by default.
>>> + *
>>> + * This is also the DR6 architectural value following Power-up, Reset or INIT.
>>> + *
>>> + * Note, with the introduction of Bus Lock Detection (BLD) and Restricted
>>> + * Transactional Memory (RTM), the DR6 register has been modified:
>>> + *
>>> + * 1) BLD flag (bit 11) is no longer reserved to 1 if the CPU supports
>>> + * Bus Lock Detection. The assertion of a bus lock could clear it.
>>> + *
>>> + * 2) RTM flag (bit 16) is no longer reserved to 1 if the CPU supports
>>> + * restricted transactional memory. #DB occurred inside an RTM region
>>> + * could clear it.
>>> + *
>>> + * Apparently, DR6.BLD and DR6.RTM are active low bits.
>>> + *
>>> + * As a result, DR6_RESERVED is an incorrect name now, but it is kept for
>>> + * compatibility.
>>> + */
>>> #define DR6_RESERVED (0xFFFF0FF0)
>>> #define DR_TRAP0 (0x1) /* db0 */
>>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>>> index 8feb8fd2957a..0f6c280a94f0 100644
>>> --- a/arch/x86/kernel/cpu/common.c
>>> +++ b/arch/x86/kernel/cpu/common.c
>>> @@ -2243,20 +2243,16 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
>>> #endif
>>> #endif
>>> -/*
>>> - * Clear all 6 debug registers:
>>> - */
>>> -static void clear_all_debug_regs(void)
>>> +static void initialize_debug_regs(void)
>>> {
>>> - int i;
>>> -
>>> - for (i = 0; i < 8; i++) {
>>> - /* Ignore db4, db5 */
>>> - if ((i == 4) || (i == 5))
>>> - continue;
>>> -
>>> - set_debugreg(0, i);
>>> - }
>>> + /* Control register first -- to make sure everything is disabled. */
>> In the Figure 19-1. Debug Registers of SDM section 19.2 DEBUG REGISTERS,
>>
>> bit 10, 12, 14, 15 of DR7 are marked as gray (Reversed) and their value are filled as
>>
>> 1, 0, 0,0 ; should we clear them all here ? I didn't find any other description in the
>>
>> SDM about the result if they are cleaned. of course, this patch doesn't change
>>
>> the behaviour of original DR7 initialization code, no justification needed,
>>
>> just out of curiosity.
>>
>>
>> Thanks,
>>
>> Ethan
>>
>>> + set_debugreg(0, 7);
>>> + set_debugreg(DR6_RESERVED, 6);
>>> + /* dr5 and dr4 don't exist */
>>> + set_debugreg(0, 3);
>>> + set_debugreg(0, 2);
>>> + set_debugreg(0, 1);
>>> + set_debugreg(0, 0);
>>> }
>>> #ifdef CONFIG_KGDB
>>> @@ -2417,7 +2413,7 @@ void cpu_init(void)
>>> load_mm_ldt(&init_mm);
>>> - clear_all_debug_regs();
>>> + initialize_debug_regs();
>>> dbg_restore_debug_regs();
>>> doublefault_init_cpu_tss();
>>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>>> index c5c897a86418..36354b470590 100644
>>> --- a/arch/x86/kernel/traps.c
>>> +++ b/arch/x86/kernel/traps.c
>>> @@ -1022,24 +1022,32 @@ static bool is_sysenter_singlestep(struct pt_regs *regs)
>>> #endif
>>> }
>>> -static __always_inline unsigned long debug_read_clear_dr6(void)
>>> +static __always_inline unsigned long debug_read_reset_dr6(void)
>>> {
>>> unsigned long dr6;
>>> + get_debugreg(dr6, 6);
>>> + dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
>>> +
>>> /*
>>> * The Intel SDM says:
>>> *
>>> - * Certain debug exceptions may clear bits 0-3. The remaining
>>> - * contents of the DR6 register are never cleared by the
>>> - * processor. To avoid confusion in identifying debug
>>> - * exceptions, debug handlers should clear the register before
>>> - * returning to the interrupted task.
>>> + * Certain debug exceptions may clear bits 0-3 of DR6.
>>> + *
>>> + * BLD induced #DB clears DR6.BLD and any other debug
>>> + * exception doesn't modify DR6.BLD.
>>> *
>>> - * Keep it simple: clear DR6 immediately.
>>> + * RTM induced #DB clears DR6.RTM and any other debug
>>> + * exception sets DR6.RTM.
>>> + *
>>> + * To avoid confusion in identifying debug exceptions,
>>> + * debug handlers should set DR6.BLD and DR6.RTM, and
>>> + * clear other DR6 bits before returning.
>>> + *
>>> + * Keep it simple: write DR6 with its architectural reset
>>> + * value 0xFFFF0FF0, defined as DR6_RESERVED, immediately.
>>> */
>>> - get_debugreg(dr6, 6);
>>> set_debugreg(DR6_RESERVED, 6);
>>> - dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
>>> return dr6;
>>> }
>>> @@ -1239,13 +1247,13 @@ static noinstr void exc_debug_user(struct pt_regs *regs, unsigned long dr6)
>>> /* IST stack entry */
>>> DEFINE_IDTENTRY_DEBUG(exc_debug)
>>> {
>>> - exc_debug_kernel(regs, debug_read_clear_dr6());
>>> + exc_debug_kernel(regs, debug_read_reset_dr6());
>>> }
>>> /* User entry, runs on regular task stack */
>>> DEFINE_IDTENTRY_DEBUG_USER(exc_debug)
>>> {
>>> - exc_debug_user(regs, debug_read_clear_dr6());
>>> + exc_debug_user(regs, debug_read_reset_dr6());
>>> }
>>> #ifdef CONFIG_X86_FRED
>>> @@ -1264,7 +1272,7 @@ DEFINE_FREDENTRY_DEBUG(exc_debug)
>>> {
>>> /*
>>> * FRED #DB stores DR6 on the stack in the format which
>>> - * debug_read_clear_dr6() returns for the IDT entry points.
>>> + * debug_read_reset_dr6() returns for the IDT entry points.
>>> */
>>> unsigned long dr6 = fred_event_data(regs);
>>> @@ -1279,7 +1287,7 @@ DEFINE_FREDENTRY_DEBUG(exc_debug)
>>> /* 32 bit does not have separate entry points. */
>>> DEFINE_IDTENTRY_RAW(exc_debug)
>>> {
>>> - unsigned long dr6 = debug_read_clear_dr6();
>>> + unsigned long dr6 = debug_read_reset_dr6();
>>> if (user_mode(regs))
>>> exc_debug_user(regs, dr6);
> We should, but it isn't a manifest bug so is slightly less urgent.
so far, yes.
Thanks,
Ethan
--
"firm, enduring, strong, and long-lived"
Powered by blists - more mailing lists