linux-kernel - Re: [PATCH v4] x86/entry_32: Use stack segment selector for VERW operand

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240710231609.rbxd7m5mjk53rthl@desk>
Date: Wed, 10 Jul 2024 16:16:09 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Uros Bizjak <ubizjak@...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>,
	Brian Gerst <brgerst@...il.com>,
	"Linux regression tracking (Thorsten Leemhuis)" <regressions@...mhuis.info>,
	antonio.gomez.iglesias@...ux.intel.com,
	daniel.sneddon@...ux.intel.com, stable@...r.kernel.org
Subject: Re: [PATCH v4] x86/entry_32: Use stack segment selector for VERW
 operand

On Wed, Jul 10, 2024 at 11:50:50PM +0200, Uros Bizjak wrote:
> 
> 
> On 10. 07. 24 21:06, Pawan Gupta wrote:
> > Robert Gill reported below #GP when dosemu software was executing vm86()
> > system call:
> > 
> >    general protection fault: 0000 [#1] PREEMPT SMP
> >    CPU: 4 PID: 4610 Comm: dosemu.bin Not tainted 6.6.21-gentoo-x86 #1
> >    Hardware name: Dell Inc. PowerEdge 1950/0H723K, BIOS 2.7.0 10/30/2010
> >    EIP: restore_all_switch_stack+0xbe/0xcf
> >    EAX: 00000000 EBX: 00000000 ECX: 00000000 EDX: 00000000
> >    ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: ff8affdc
> >    DS: 0000 ES: 0000 FS: 0000 GS: 0033 SS: 0068 EFLAGS: 00010046
> >    CR0: 80050033 CR2: 00c2101c CR3: 04b6d000 CR4: 000406d0
> >    Call Trace:
> >     show_regs+0x70/0x78
> >     die_addr+0x29/0x70
> >     exc_general_protection+0x13c/0x348
> >     exc_bounds+0x98/0x98
> >     handle_exception+0x14d/0x14d
> >     exc_bounds+0x98/0x98
> >     restore_all_switch_stack+0xbe/0xcf
> >     exc_bounds+0x98/0x98
> >     restore_all_switch_stack+0xbe/0xcf
> > 
> > This only happens when VERW based mitigations like MDS/RFDS are enabled.
> > This is because segment registers with an arbitrary user value can result
> > in #GP when executing VERW. Intel SDM vol. 2C documents the following
> > behavior for VERW instruction:
> > 
> >    #GP(0) - If a memory operand effective address is outside the CS, DS, ES,
> > 	   FS, or GS segment limit.
> > 
> > CLEAR_CPU_BUFFERS macro executes VERW instruction before returning to user
> > space. Replace CLEAR_CPU_BUFFERS with a safer version that uses %ss to
> > refer VERW operand mds_verw_sel. This ensures VERW will not #GP for an
> > arbitrary user %ds. Also, in NMI return path, move VERW to after
> > RESTORE_ALL_NMI that touches GPRs.
> > 
> > For clarity, below are the locations where the new CLEAR_CPU_BUFFERS_SAFE
> > version is being used:
> > 
> > * entry_INT80_32(), entry_SYSENTER_32() and interrupts (via
> >    handle_exception_return) do:
> > 
> > restore_all_switch_stack:
> >    [...]
> >     mov    %esi,%esi
> >     verw   %ss:0xc0fc92c0  <-------------
> >     iret
> > 
> > * Opportunistic SYSEXIT:
> > 
> >     [...]
> >     verw   %ss:0xc0fc92c0  <-------------
> >     btrl   $0x9,(%esp)
> >     popf
> >     pop    %eax
> >     sti
> >     sysexit
> > 
> > *  nmi_return and nmi_from_espfix:
> >     mov    %esi,%esi
> >     verw   %ss:0xc0fc92c0  <-------------
> >     jmp     .Lirq_return
> > 
> > Fixes: a0e2dab44d22 ("x86/entry_32: Add VERW just before userspace transition")
> > Cc: stable@...r.kernel.org # 5.10+
> > Reported-by: Robert Gill <rtgill82@...il.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218707
> > Closes: https://lore.kernel.org/all/8c77ccfd-d561-45a1-8ed5-6b75212c7a58@leemhuis.info/
> > Suggested-by: Dave Hansen <dave.hansen@...ux.intel.com>
> > Suggested-by: Brian Gerst <brgerst@...il.com> # Use %ss
> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> > ---
> > v4:
> > - Further simplify the patch by using %ss for all VERW calls in 32-bit mode (Brian).
> > - In NMI exit path move VERW after RESTORE_ALL_NMI that touches GPRs (Dave).
> > 
> > v3: https://lore.kernel.org/r/20240701-fix-dosemu-vm86-v3-1-b1969532c75a@linux.intel.com
> > - Simplify CLEAR_CPU_BUFFERS_SAFE by using %ss instead of %ds (Brian).
> > - Do verw before popf in SYSEXIT path (Jari).
> > 
> > v2: https://lore.kernel.org/r/20240627-fix-dosemu-vm86-v2-1-d5579f698e77@linux.intel.com
> > - Safe guard against any other system calls like vm86() that might change %ds (Dave).
> > 
> > v1: https://lore.kernel.org/r/20240426-fix-dosemu-vm86-v1-1-88c826a3f378@linux.intel.com
> > ---
> > 
> > ---
> >   arch/x86/entry/entry_32.S | 18 +++++++++++++++---
> >   1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> > index d3a814efbff6..d54f6002e5a0 100644
> > --- a/arch/x86/entry/entry_32.S
> > +++ b/arch/x86/entry/entry_32.S
> > @@ -253,6 +253,16 @@
> >   .Lend_\@:
> >   .endm
> > +/*
> > + * Safer version of CLEAR_CPU_BUFFERS that uses %ss to reference VERW operand
> > + * mds_verw_sel. This ensures VERW will not #GP for an arbitrary user %ds.
> > + */
> > +.macro CLEAR_CPU_BUFFERS_SAFE
> > +	ALTERNATIVE "jmp .Lskip_verw\@", "", X86_FEATURE_CLEAR_CPU_BUF
> > +	verw	%ss:_ASM_RIP(mds_verw_sel)
> > +.Lskip_verw\@:
> > +.endm
> 
> Why not simply:
> 
> .macro CLEAR_CPU_BUFFERS_SAFE
> 	ALTERNATIVE "", __stringify(verw %ss:_ASM_RIP(mds_verw_sel)),
> X86_FEATURE_CLEAR_CPU_BUF
> .endm

We can do it this way as well. But, there are stable kernels that don't
support relocations in ALTERNATIVEs. The way it is done in current patch
can be backported without worrying about which kernels support relocations.