linux-kernel - Re: [PATCH v3] x86/entry_32: Move CLEAR_CPU

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240701232221.s4egkkyctgrasulr@desk>
Date: Mon, 1 Jul 2024 16:22:21 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Brian Gerst <brgerst@...il.com>
Cc: Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	linux-kernel@...r.kernel.org, x86@...nel.org,
	Robert Gill <rtgill82@...il.com>,
	Jari Ruusu <jariruusu@...tonmail.com>,
	"Linux regression tracking (Thorsten Leemhuis)" <regressions@...mhuis.info>,
	antonio.gomez.iglesias@...ux.intel.com,
	daniel.sneddon@...ux.intel.com
Subject: Re: [PATCH v3] x86/entry_32: Move CLEAR_CPU_BUFFERS before restoring
 segments

On Mon, Jul 01, 2024 at 03:22:44PM -0400, Brian Gerst wrote:
> Perhaps I should have been a bit clearer, but I meant adding the SS
> override to the VERW instructions in their present locations, instead
> of moving it into RESTORE_REGS.

I did consider that way, but moving it to RESTORE_REGS saves a callsite for
CLEAR_CPU_BUFFERS. I believe Dave prefered it within RESTORE_REGS, but that
was before the SS override. Unless anyone has a strong preference, I will
change the current callsites to include the SS override, and not move
CLEAR_CPU_BUFFERS to RESTORE_REGS.

> IIRC we don't want data reads (including stack pops) between the VERW
> instruction and IRET/SYSEXIT.

Stack pops are okay if the memory *and* the registers doesn't contain
sensitive data, which doesn't seem to be the case here. But, SS override is
a safer option.

BTW, Dave pointed out that CLEAR_CPU_BUFFERS needs to be done after
RESTORE_ALL_NMI in the NMI return path, that adds an extra callsite:

---
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d3a814efbff6..9b58d2cbdfef 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1144,7 +1144,6 @@ SYM_CODE_START(asm_exc_nmi)
 
 	/* Not on SYSENTER stack. */
 	call	exc_nmi
-	CLEAR_CPU_BUFFERS
 	jmp	.Lnmi_return
 
 .Lnmi_from_sysenter_stack:
@@ -1165,6 +1164,7 @@ SYM_CODE_START(asm_exc_nmi)
 
 	CHECK_AND_APPLY_ESPFIX
 	RESTORE_ALL_NMI cr3_reg=%edi pop=4
+	CLEAR_CPU_BUFFERS
 	jmp	.Lirq_return
 
 #ifdef CONFIG_X86_ESPFIX32
@@ -1206,6 +1206,7 @@ SYM_CODE_START(asm_exc_nmi)
 	 *  1 - orig_ax
 	 */
 	lss	(1+5+6)*4(%esp), %esp			# back to espfix stack
+	CLEAR_CPU_BUFFERS
 	jmp	.Lirq_return
 #endif
 SYM_CODE_END(asm_exc_nmi)