Received-SPF: pass (google.com: domain of linux-kernel+bounces-175026-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 9 May 2024 15:17:17 -0700
From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	Robert Gill <rtgill82@gmail.com>,
	"Linux regression tracking (Thorsten Leemhuis)" <regressions@leemhuis.info>,
	antonio.gomez.iglesias@linux.intel.com,
	daniel.sneddon@linux.intel.com
Subject: Re: [PATCH] x86/entry_32: Move CLEAR_CPU_BUFFERS before CR3 switch
Message-ID: <20240509221425.zcl6c45thb7wxyza@desk>
References: <20240426-fix-dosemu-vm86-v1-1-88c826a3f378@linux.intel.com>
 <5b5e597d-7620-4a5a-9bfa-bae26f0b0fa3@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5b5e597d-7620-4a5a-9bfa-bae26f0b0fa3@intel.com>

On Thu, May 09, 2024 at 09:14:01AM -0700, Dave Hansen wrote:
> On 4/26/24 16:48, Pawan Gupta wrote:
> > As the mitigation for MDS and RFDS, CLEAR_CPU_BUFFERS macro executes VERW
> > instruction that is used to clear the CPU buffers before returning to user
> > space. Currently, VERW is executed after the user CR3 is restored. This
> > leads to vm86() to fault because VERW takes a memory operand that is not
> > mapped in user page tables when vm86() syscall returns. This is an issue
> > with 32-bit kernels only, as 64-bit kernels do not support vm86().
> 
> entry.S has this handy comment:
> 
>  /*
>   * Define the VERW operand that is disguised as entry code so that
>   * it can be referenced with KPTI enabled. This ensure VERW can be
>   * used late in exit-to-user path after page tables are switched.
>   */
> 
> Why isn't that working?

It works in general, but not for vm86() syscall. I don't know much about
how vm86() works, but it seems to emulate 16-bit real mode with limited
memory mapped in user page table. Most likely, user page table doesn't have
a mapping for mds_ver_sel is not mapped resulting in #GP fault.

[...]
> Right now, this code basically does:
> 
> 	STACKLEAK_ERASE
> 	/* Restore user registers and segments */
> 	movl    PT_EIP(%esp), %edx
> 	...
> 	SWITCH_TO_USER_CR3 scratch_reg=%eax
> 	...
> 	CLEAR_CPU_BUFFERS
> 
> The proposed patch is:
> 
> 	STACKLEAK_ERASE
> +	CLEAR_CPU_BUFFERS
> 	/* Restore user registers and segments */
> 	movl    PT_EIP(%esp), %edx
> 	...
> 	SWITCH_TO_USER_CR3 scratch_reg=%eax
> 	...
> -	CLEAR_CPU_BUFFERS
> 
> That's a bit confusing to me.  I would have expected the
> CLEAR_CPU_BUFFERS to go _just_ before the SWITCH_TO_USER_CR3 and after
> the user register restore.
> 
> Is there a reason it can't go there?  I think only %eax is "live" with
> kernel state at that point and it's only an entry stack pointer, so not
> a secret.

It probably can go right before the SWITCH_TO_USER_CR3. I didn't have
32-bit setup with dosemu to experiment with. I will attach a debug patch to
the bugzilla and request the reporter to test it.

> >  	/*
> >  	 * Return back to the vDSO, which will pop ecx and edx.
> > @@ -941,6 +941,7 @@ SYM_FUNC_START(entry_INT80_32)
> >  	STACKLEAK_ERASE
> >  
> >  restore_all_switch_stack:
> > +	CLEAR_CPU_BUFFERS
> >  	SWITCH_TO_ENTRY_STACK
> >  	CHECK_AND_APPLY_ESPFIX
> >  
> > @@ -951,7 +952,6 @@ restore_all_switch_stack:
> >  
> >  	/* Restore user state */
> >  	RESTORE_REGS pop=4			# skip orig_eax/error_code
> > -	CLEAR_CPU_BUFFERS
> >  .Lirq_return:
> >  	/*
> >  	 * ARCH_HAS_MEMBARRIER_SYNC_CORE rely on IRET core serialization
> 
> There is a working stack here, on both sides of the CR3 switch.  It's
> annoying to do another push/pop which won't get patched out, but this
> _could_ just do:
> 
> 	RESTORE_REGS pop=4
> 	CLEAR_CPU_BUFFERS
> 
> 	pushl %eax
> 	SWITCH_TO_USER_CR3 scratch_reg=%eax
> 	popl %eax
> 
> right?

We can probably avoid the push/pop as well, because CLEAR_CPU_BUFFERS will
only clobber the ZF.

> That would only expose the CR3 value, which isn't a secret.

Right.