Received-SPF: pass (google.com: domain of linux-kernel+bounces-137699-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <56cbf1f0-0867-44c2-a9fd-712bf3549cbb@zytor.com>
Date: Tue, 9 Apr 2024 16:35:35 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/1] x86/entry/64: Skip SYSRET validation tests when
 FRED is enabled
Content-Language: en-US
To: "Xin Li (Intel)" <xin@zytor.com>, linux-kernel@vger.kernel.org
Cc: luto@kernel.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, brgerst@gmail.com
References: <20240403062404.2956240-1-xin@zytor.com>
From: "H. Peter Anvin" <hpa@zytor.com>
In-Reply-To: <20240403062404.2956240-1-xin@zytor.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Now, for a FRED system the return value is ignored anyway, so the only 
benefit of this is skipping the test, correct?

I do observe than what is left of do_fast_syscall_32 is a single 
assignment followed by __do_fast_syscall_32(), which could be turned 
into a tailcall.

Another thing that has been added lately is a bunch of tests on the int 
$0x80 path. This is a slow path *on legacy hardware*, but on FRED 
systems it is the *fast* path for IA32 emulation. These tests are also 
totally unnecessary *AND IN FACT, WRONG* on a FRED system, as FRED 
distinguishes external interrupts from software interrupts.

Furthermore, under FRED interrupt 0x80 is available as a hardware 
interrupt (since there is no reason to block it out.) Therefore, going 
and poking the APIC as in int80_is_external() is INCORRECT and possibly 
fatal.

Again, the easiest way to fix that is to follow what XenPV does in 
int80_is_external(), but there is more unnecessary stuff: 
!user_mode(regs) cannot happen on FRED, and the stuff in the 
int80_emulation assembly function should, if it is needed at all on any 
FRED-compatible hardware (I don't believe so) should be done in the FRED 
user mode assembly entry stub.

Thus, it might be better to strip down do_int80_emulation() to a lean 
fred_int80_emulation().

	-hpa

On 4/2/24 23:24, Xin Li (Intel) wrote:
> Don't do SYSRET validation tests when FRED is enabled, since ERETU is
> the only legit instruction to return to user level.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
>   arch/x86/entry/common.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 6356060caaf3..1c3944eb9901 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -72,7 +72,7 @@ static __always_inline bool do_syscall_x32(struct pt_regs *regs, int nr)
>   	return false;
>   }
>   
> -/* Returns true to return using SYSRET, or false to use IRET */
> +/* Returns true to return using SYSRET, or false to use IRET/ERETU */
>   __visible noinstr bool do_syscall_64(struct pt_regs *regs, int nr)
>   {
>   	add_random_kstack_offset();
> @@ -88,6 +88,10 @@ __visible noinstr bool do_syscall_64(struct pt_regs *regs, int nr)
>   	instrumentation_end();
>   	syscall_exit_to_user_mode(regs);
>   
> +	/* No test for FRED, which returns to user level with ERETU only */
> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
> +		return false;
> +
>   	/*
>   	 * Check that the register state is valid for using SYSRET to exit
>   	 * to userspace.  Otherwise use the slower but fully capable IRET
> @@ -325,7 +329,7 @@ static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
>   	return true;
>   }
>   
> -/* Returns true to return using SYSEXIT/SYSRETL, or false to use IRET */
> +/* Returns true to return using SYSEXIT/SYSRETL, or false to use IRET/ERETU */
>   __visible noinstr bool do_fast_syscall_32(struct pt_regs *regs)
>   {
>   	/*
> @@ -346,6 +350,10 @@ __visible noinstr bool do_fast_syscall_32(struct pt_regs *regs)
>   	if (!__do_fast_syscall_32(regs))
>   		return false;
>   
> +	/* No test for FRED, which returns to user level with ERETU only */
> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
> +		return false;
> +
>   	/*
>   	 * Check that the register state is valid for using SYSRETL/SYSEXIT
>   	 * to exit to userspace.  Otherwise use the slower but fully capable
> 
> base-commit: 65d1240b6728b38e4d2068d6738a17e4ee4351f5