[PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[Xin Li (Intel)]

Add a FRED-specific INT80 handler fred_int80_emulation().

Commit
  55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
added a bunch of checks to the int $0x80 path, however they are
unnecessary and event wrong in fact under FRED.

1) FRED distinguishes external interrupts from software interrupts,
   thus int80_emulation() should NEVER be called for handling an
   external interrupt, and then int80_is_external() is meaningless
   when FRED is enabled.

2) The FRED kernel entry handler NEVER dispatches INTx, which is
   of event type EVENT_TYPE_SWINT, so the user mode checking in
   do_int80_emulation() is redundant.

3) int80_emulation() does a CLEAR_BRANCH_HISTORY, which is likly
   an overkill for new x86 CPU implementations that support FRED.

A dedicated FRED INT80 handler duplicates most of the code in
do_int80_emulation(), but it avoids sprinkling moar tests and
seems more readable.

Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---

Change since v1:
* Prefer a FRED-specific INT80 handler instead of sprinkling moar
  tests around (Borislav Petkov).
---
 arch/x86/entry/common.c     | 40 +++++++++++++++++++++++++++++++++++++
 arch/x86/entry/entry_fred.c |  2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6de50b80702e..3cfc7c4fe1a2 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -255,6 +255,46 @@ __visible noinstr void do_int80_emulation(struct pt_regs *regs)
 	instrumentation_end();
 	syscall_exit_to_user_mode(regs);
 }
+
+#ifdef CONFIG_X86_FRED
+/*
+ * Called only if a user level application executes "int $0x80".
+ *
+ * Note, under FRED interrupt 0x80 could be used as a hardware interrupt.
+ */
+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
-- 
2.44.0

Re: [PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[H. Peter Anvin]

On 4/16/24 10:58, Xin Li (Intel) wrote:
> Add a FRED-specific INT80 handler fred_int80_emulation().
> 
> Commit
>    55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
> added a bunch of checks to the int $0x80 path, however they are
> unnecessary and event wrong in fact under FRED.


I think the following points should be added to the head comment, and 
not just the commit log:

> 1) FRED distinguishes external interrupts from software interrupts,
>     thus int80_emulation() should NEVER be called for handling an
>     external interrupt, and then int80_is_external() is meaningless
>     when FRED is enabled.

1a) 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 could cause a system call to be incorrectly 
ignored.

> 2) The FRED kernel entry handler NEVER dispatches INTx, which is
>     of event type EVENT_TYPE_SWINT, so the user mode checking in
>     do_int80_emulation() is redundant.
> 
> 3) int80_emulation() does a CLEAR_BRANCH_HISTORY, which is likly
s/likly/likely/
>     an overkill for new x86 CPU implementations that support FRED.
s/an //

4) int $0x80 is the FAST path for 32-bit system calls under FRED.

[...]

> A dedicated FRED INT80 handler duplicates most of the code in
s/most/quite a bit/

(I think there is actually less than half the code left. This could be 
further cleaned up by inlining the common code, but if I were still 
maintainer I would not want that for x86/urgent. This patch has the very 
nice property for x86/urgent purposes that it doesn't touch non-FRED 
code at all.)

> do_int80_emulation(), but it avoids sprinkling moar tests and
s/moar/more/
> seems more readable.
> 
> Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>

So, as Borislav pointed out:

Fixes: 55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")

My fault.

Re: [PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[Borislav Petkov]

On Tue, Apr 16, 2024 at 11:23:24AM -0700, H. Peter Anvin wrote:
> (I think there is actually less than half the code left. This could be
> further cleaned up by inlining the common code,

Yeah, was just thinking about that too. I'd say, though, let's let the
FRED code settle, we start using it and then we can always unify common
stuff later if it turns out that it won't diverge anymore.

> but if I were still maintainer I would not want that for x86/urgent.
> This patch has the very nice property for x86/urgent purposes that it
> doesn't touch non-FRED code at all.)

Right.

I'd let Xin work in all those comments and then I'll pick it up.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[H. Peter Anvin]

On 4/16/24 11:28, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:23:24AM -0700, H. Peter Anvin wrote:
>> (I think there is actually less than half the code left. This could be
>> further cleaned up by inlining the common code,
> 
> Yeah, was just thinking about that too. I'd say, though, let's let the
> FRED code settle, we start using it and then we can always unify common
> stuff later if it turns out that it won't diverge anymore.

Good plan.

>> but if I were still maintainer I would not want that for x86/urgent.
>> This patch has the very nice property for x86/urgent purposes that it
>> doesn't touch non-FRED code at all.)
> 
> Right.

:)

> I'd let Xin work in all those comments and then I'll pick it up.

Thanks!

	-hpa

Re: [PATCH v2 1/1] x86/fred: Fix INT80 emulation for FRED

[Xin Li]

On 4/16/2024 11:33 AM, H. Peter Anvin wrote:
> On 4/16/24 11:28, Borislav Petkov wrote:
>> On Tue, Apr 16, 2024 at 11:23:24AM -0700, H. Peter Anvin wrote:
>>> (I think there is actually less than half the code left. This could be
>>> further cleaned up by inlining the common code,
>>
>> Yeah, was just thinking about that too. I'd say, though, let's let the
>> FRED code settle, we start using it and then we can always unify common
>> stuff later if it turns out that it won't diverge anymore.
> 
> Good plan.

This is a nice tip to me, because I did feel a bit of guilty that I 
didn't use an inline common function, although I didn't think that
is really necessary.

Thanks!
     Xin