2024-04-17 06:30:44

by Xin Li (Intel)

[permalink] [raw]
Subject: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

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.

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) <[email protected]>
Signed-off-by: Xin Li (Intel) <[email protected]>
---

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
--
2.44.0



2024-04-17 09:39:21

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On Tue, Apr 16, 2024 at 11:30:01PM -0700, Xin Li (Intel) wrote:
> 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.

What does that mean?

An event handler doesn't dispatch INT insns?

/me is confused.

> +#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.
> + */

And this is talking about duplication above and that text is duplicated
from the commit message. :)

I'll zap it when applying.

Thx.

--
Regards/Gruss,
Boris.

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

2024-04-17 11:05:02

by Nikolay Borisov

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED



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) <[email protected]>
> Signed-off-by: Xin Li (Intel) <[email protected]>
> ---
>
> 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

2024-04-17 15:00:07

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/24 02:38, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:30:01PM -0700, Xin Li (Intel) wrote:
>> 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.
>
> What does that mean?
>
> An event handler doesn't dispatch INT insns?
>
> /me is confused.

FRED has separate entry flows depending on if the event came from user
space or kernel space:

asm_fred_entrypoint_user -> fred_entry_from_user

asm_fred_entrypoint_kernel -> fred_entry_from_kernel

fred_entry_from_kernel does not invoke fred_intx() if the event type is
EVENT_TYPE_SWINT, instead it falls through to fred_bad_type(). Perhaps
fred_intx() should be renamed fred_intx_user() for additional clarity.

(It might also we worth noting in that function that the reason int
$0x03 and int $0x04 are dispatched as INT3 and INTO is to be fully user
space compatible with legacy IDT, which behaves similarly.)

Thus, the int $0x80 code is simply not reachable from kernel space; if
kernel code were to invoke int $0x80 or any other INT instruction it
will error out before getting to this code.

>> +#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.
>> + */
>
> And this is talking about duplication above and that text is duplicated
> from the commit message. :)
>
> I'll zap it when applying.
>

I suggested putting it into a comment for future reference. Obviously no
need to duplicate it in the commit message :)

-hpa


2024-04-17 15:07:11

by Xin Li (Intel)

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/2024 2:38 AM, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:30:01PM -0700, Xin Li (Intel) wrote:
>> 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.
>
> What does that mean?
>
> An event handler doesn't dispatch INT insns?
>
> /me is confused.
>

Maybe better to say?

The FRED kernel entry handler fred_entry_from_kernel() calls
fred_bad_type() if an event is of type EVENT_TYPE_SWINT because Kernel
does not use INT insns! So if the kernel is handling an INT insn, it can
only be from a user level application.


>> + * 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.
>> + */
>
> And this is talking about duplication above and that text is duplicated
> from the commit message. :)

I wanted to keep it as a TODO. I can remove it in the next iteration.

>
> I'll zap it when applying.
>
> Thx.
>


2024-04-17 15:10:25

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/24 08:06, Xin Li wrote:
> On 4/17/2024 2:38 AM, Borislav Petkov wrote:
>> On Tue, Apr 16, 2024 at 11:30:01PM -0700, Xin Li (Intel) wrote:
>>> 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.
>>
>> What does that mean?
>>
>> An event handler doesn't dispatch INT insns?
>>
>> /me is confused.
>>
>
> Maybe better to say?
>
> The FRED kernel entry handler fred_entry_from_kernel() calls
> fred_bad_type() if an event is of type EVENT_TYPE_SWINT because Kernel
> does not use INT insns! So if the kernel is handling an INT insn, it can
> only be from a user level application.
>

(Note: if we at some point in the future we may end up adding INT calls
from the kernel, at least on FRED it would *still* not share the same
code paths and so int $0x80 would not get dispatched.)

-hpa

2024-04-17 15:29:59

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/24 04:02, Nikolay Borisov wrote:
>
> 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).
>>

It would seem like a huge stretch to expect that a FRED-capable CPU
would not have such a facility. This is a matter of establishing a
baseline for FRED-capable hardware.

It would make more sense to me to add it if we turn out to need it; note
that FRED code is currently only enabled on demand, in order to defend
against bit rot until we have physical hardware.

Now, if this is still desired, it *probably* belongs better in either
fred_intx()/fred_other() or asm_fred_entrypoint_user, depending on if
this ought to be done for all entries from userspace or only system calls.

-hpa

2024-04-17 15:35:17

by Xin Li (Intel)

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/2024 7:59 AM, H. Peter Anvin wrote:
> On 4/17/24 02:38, Borislav Petkov wrote:
>> On Tue, Apr 16, 2024 at 11:30:01PM -0700, Xin Li (Intel) wrote:
>>> 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.
>>
>> What does that mean?
>>
>> An event handler doesn't dispatch INT insns?
>>
>> /me is confused.
>
> FRED has separate entry flows depending on if the event came from user
> space or kernel space:
>
> asm_fred_entrypoint_user -> fred_entry_from_user
>
> asm_fred_entrypoint_kernel -> fred_entry_from_kernel
>
> fred_entry_from_kernel does not invoke fred_intx() if the event type is
> EVENT_TYPE_SWINT, instead it falls through to fred_bad_type(). Perhaps
> fred_intx() should be renamed fred_intx_user() for additional clarity.

This is a good idea, and again naming is so important.

>
> (It might also we worth noting in that function that the reason int
> $0x03 and int $0x04 are dispatched as INT3 and INTO is to be fully user
> space compatible with legacy IDT, which behaves similarly.)

Yeah, this is subtle, and we'd better make it explicit with comments.

FRED distinguishes int $0x03/$0x04 from INT3/INTO with event type
EVENT_TYPE_SWINT and EVENT_TYPE_SWEXC, and the Linux kernel itself can
still use INT3/INTO, however int $0x03/$0x04 is NOT allowed from kernel
context.

>
> Thus, the int $0x80 code is simply not reachable from kernel space; if
> kernel code were to invoke int $0x80 or any other INT instruction it
> will error out before getting to this code.
>
>>> +#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.
>>> + */
>>
>> And this is talking about duplication above and that text is duplicated
>> from the commit message. :)
>>
>> I'll zap it when applying.
>>
>
> I suggested putting it into a comment for future reference. Obviously no
> need to duplicate it in the commit message :)
>

yes!


2024-04-17 15:56:42

by Xin Li (Intel)

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On 4/17/2024 8:07 AM, H. Peter Anvin wrote:
> On 4/17/24 04:02, Nikolay Borisov wrote:
>>
>> On 17.04.24 г. 9:30 ч., Xin Li (Intel) wrote:
>>> 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).
>>>
>
> It would seem like a huge stretch to expect that a FRED-capable CPU
> would not have such a facility. This is a matter of establishing a
> baseline for FRED-capable hardware.
>
> It would make more sense to me to add it if we turn out to need it; note
> that FRED code is currently only enabled on demand, in order to defend
> against bit rot until we have physical hardware.
>
> Now, if this is still desired, it *probably* belongs better in either
> fred_intx()/fred_other() or asm_fred_entrypoint_user, depending on if
> this ought to be done for all entries from userspace or only system calls.

My bad that I didn't make this a good comment, even neglected that
clear_branch_history is just a nop on machines w/o such security issues.


So how about?

int80_emulation() does a CLEAR_BRANCH_HISTORY, which is IDT-specific.
While FRED will likely take a different approach when it is needed:
it *probably* belongs in either fred_intx()/fred_other() or
asm_fred_entrypoint_user(), depending on if this ought to be done for
all entries from userspace or only system calls.

2024-04-17 16:40:23

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED



On 4/17/24 08:55, Xin Li wrote:
> On 4/17/2024 8:07 AM, H. Peter Anvin wrote:
>> On 4/17/24 04:02, Nikolay Borisov wrote:
>>>
>>> On 17.04.24 г. 9:30 ч., Xin Li (Intel) wrote:
>>>> 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).
>>>>
>>
>> It would seem like a huge stretch to expect that a FRED-capable CPU
>> would not have such a facility. This is a matter of establishing a
>> baseline for FRED-capable hardware.
>>
>> It would make more sense to me to add it if we turn out to need it;
>> note that FRED code is currently only enabled on demand, in order to
>> defend against bit rot until we have physical hardware.
>>
>> Now, if this is still desired, it *probably* belongs better in either
>> fred_intx()/fred_other() or asm_fred_entrypoint_user, depending on if
>> this ought to be done for all entries from userspace or only system
>> calls.
>
> My bad that I didn't make this a good comment, even neglected that
> clear_branch_history is just a nop on machines w/o such security issues.
>
>
> So how about?
>
> int80_emulation() does a CLEAR_BRANCH_HISTORY, which is IDT-specific.
> While FRED will likely take a different approach when it is needed:

s/when/if it is ever needed/

> it *probably* belongs in either fred_intx()/fred_other() or
> asm_fred_entrypoint_user(), depending on if this ought to be done for
> all entries from userspace or only system calls.

2024-04-17 20:29:13

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] x86/fred: Fix INT80 emulation for FRED

On Wed, Apr 17, 2024 at 07:59:25AM -0700, H. Peter Anvin wrote:
> FRED has separate entry flows depending on if the event came from user space
> or kernel space:
>
> asm_fred_entrypoint_user -> fred_entry_from_user
>
> asm_fred_entrypoint_kernel -> fred_entry_from_kernel
>
> fred_entry_from_kernel does not invoke fred_intx() if the event type is
> EVENT_TYPE_SWINT, instead it falls through to fred_bad_type(). Perhaps
> fred_intx() should be renamed fred_intx_user() for additional clarity.

Sure.

> (It might also we worth noting in that function that the reason int $0x03
> and int $0x04 are dispatched as INT3 and INTO is to be fully user space
> compatible with legacy IDT, which behaves similarly.)

Also yes.

> Thus, the int $0x80 code is simply not reachable from kernel space; if
> kernel code were to invoke int $0x80 or any other INT instruction it will
> error out before getting to this code.

Yah, that explanation is a lot better, thanks. I'll use it. :)

> I suggested putting it into a comment for future reference. Obviously
> no need to duplicate it in the commit message :)

Yap, that makes more sense.

Thx.

--
Regards/Gruss,
Boris.

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