| 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: <7C732492-F1F9-403B-A722-9EA563795B1B@zytor.com>
Date: Mon, 23 Jun 2025 01:14:35 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Ethan Zhao <haifeng.zhao@...ux.intel.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
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.
Powered by blists - more mailing lists