Received-SPF: pass (google.com: domain of linux-kernel+bounces-148364-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <d0979bd9-fd12-4672-b451-23f23fc2353c@suse.com>
Date: Wed, 17 Apr 2024 14:02:33 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED
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, hpa@zytor.com
References: <20240417063001.3773507-1-xin@zytor.com>
From: Nikolay Borisov <nik.borisov@suse.com>
Content-Language: en-US
In-Reply-To: <20240417063001.3773507-1-xin@zytor.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 17.04.24 г. 9:30 ч., Xin Li (Intel) wrote:
> Add a FRED-specific INT80 handler fred_int80_emulation():
> 
> 1) As INT instructions and hardware interrupts are separate event
>     types, FRED does not preclude the use of vector 0x80 for external
>     interrupts. As a result the FRED setup code does *NOT* reserve
>     vector 0x80 and calling int80_is_external() is not merely
>     suboptimal but actively incorrect: it could cause a system call
>     to be incorrectly ignored.
> 
> 2) fred_int80_emulation(), only called for handling vector 0x80 of
>     event type EVENT_TYPE_SWINT, will NEVER be called to handle any
>     external interrupt (event type EVENT_TYPE_EXTINT).
> 
> 3) The FRED kernel entry handler does *NOT* dispatch INT instructions,
>     which is of event type EVENT_TYPE_SWINT, so compared with
>     do_int80_emulation(), there is no need to do any user mode check.
> 
> 4) int80_emulation() does a CLEAR_BRANCH_HISTORY, which is likely >     overkill for new x86 CPU implementations that support FRED.

Well, that's a bit of an overstatement/speculation, because 
clear_branch_history will only be effective if the machine is 
susceptible to the given bug and there isn't a better options (i.e using 
a hardware bit controlling the respective aspect of the CPU).
> 
> 5) int $0x80 is the FAST path for 32-bit system calls under FRED.
> 
> A dedicated FRED INT80 handler duplicates quite a bit of the code in
> do_int80_emulation(), but it avoids sprinkling more tests and seems
> more readable. Just remember that we can always unify common stuff
> later if it turns out that it won't diverge anymore, i.e., after the
> FRED code settles.
> 
> Fixes: 55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
> 
> Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Changes since v2:
> * Add comments explaining the reasons why a FRED-specific INT80 handler
>    is required to the head comment of fred_int80_emulation(), not just
>    the change log (H. Peter Anvin).
> * Incorporate extra clarifications from H. Peter Anvin.
> * Fix a few typos and wordings (H. Peter Anvin).
> * Add a maintainer tip to the change log and head comment: unify common
>    stuff later, i.e., after the code settles (Borislav Petkov).
> 
> Change since v1:
> * Prefer a FRED-specific INT80 handler instead of sprinkling more tests
>    around (Borislav Petkov).
> ---
>   arch/x86/entry/common.c     | 64 +++++++++++++++++++++++++++++++++++++
>   arch/x86/entry/entry_fred.c |  2 +-
>   2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 6de50b80702e..213d9b33a63c 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -255,6 +255,70 @@ __visible noinstr void do_int80_emulation(struct pt_regs *regs)
>   	instrumentation_end();
>   	syscall_exit_to_user_mode(regs);
>   }
> +
> +#ifdef CONFIG_X86_FRED
> +/*
> + * A FRED-specific INT80 handler fred_int80_emulation() is required:
> + *
> + * 1) As INT instructions and hardware interrupts are separate event
> + *    types, FRED does not preclude the use of vector 0x80 for external
> + *    interrupts. As a result the FRED setup code does *NOT* reserve
> + *    vector 0x80 and calling int80_is_external() is not merely
> + *    suboptimal but actively incorrect: it could cause a system call
> + *    to be incorrectly ignored.
> + *
> + * 2) fred_int80_emulation(), only called for handling vector 0x80 of
> + *    event type EVENT_TYPE_SWINT, will NEVER be called to handle any
> + *    external interrupt (event type EVENT_TYPE_EXTINT).
> + *
> + * 3) The FRED kernel entry handler does *NOT* dispatch INT instructions,
> + *    which is of event type EVENT_TYPE_SWINT, so compared with
> + *    do_int80_emulation(), there is no need to do any user mode check.
> + *
> + * 4) int80_emulation() does a CLEAR_BRANCH_HISTORY, which is likely
> + *    overkill for new x86 CPU implementations that support FRED.
> + *
> + * 5) int $0x80 is the FAST path for 32-bit system calls under FRED.
> + *
> + * A dedicated FRED INT80 handler duplicates quite a bit of the code in
> + * do_int80_emulation(), but it avoids sprinkling more tests and seems
> + * more readable. Just remember that we can always unify common stuff
> + * later if it turns out that it won't diverge anymore, i.e., after the
> + * FRED code settles.
> + */
> +DEFINE_FREDENTRY_RAW(int80_emulation)
> +{
> +	int nr;
> +
> +	enter_from_user_mode(regs);
> +
> +	instrumentation_begin();
> +	add_random_kstack_offset();
> +
> +	/*
> +	 * FRED pushed 0 into regs::orig_ax and regs::ax contains the
> +	 * syscall number.
> +	 *
> +	 * User tracing code (ptrace or signal handlers) might assume
> +	 * that the regs::orig_ax contains a 32-bit number on invoking
> +	 * a 32-bit syscall.
> +	 *
> +	 * Establish the syscall convention by saving the 32bit truncated
> +	 * syscall number in regs::orig_ax and by invalidating regs::ax.
> +	 */
> +	regs->orig_ax = regs->ax & GENMASK(31, 0);
> +	regs->ax = -ENOSYS;
> +
> +	nr = syscall_32_enter(regs);
> +
> +	local_irq_enable();
> +	nr = syscall_enter_from_user_mode_work(regs, nr);
> +	do_syscall_32_irqs_on(regs, nr);
> +
> +	instrumentation_end();
> +	syscall_exit_to_user_mode(regs);
> +}
> +#endif
>   #else /* CONFIG_IA32_EMULATION */
>   
>   /* Handles int $0x80 on a 32bit kernel */
> diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
> index ac120cbdaaf2..9fa18b8c7f26 100644
> --- a/arch/x86/entry/entry_fred.c
> +++ b/arch/x86/entry/entry_fred.c
> @@ -66,7 +66,7 @@ static noinstr void fred_intx(struct pt_regs *regs)
>   	/* INT80 */
>   	case IA32_SYSCALL_VECTOR:
>   		if (ia32_enabled())
> -			return int80_emulation(regs);
> +			return fred_int80_emulation(regs);
>   		fallthrough;
>   #endif
>   
> 
> base-commit: 367dc2b68007e8ca00a0d8dc9afb69bff5451ae7