2023-06-16 13:34:46

by Nikolay Borisov

[permalink] [raw]
Subject: [PATCH v3 5/5] x86/entry: Make IA32 syscalls' availability depend on ia32_enabled()

Another major aspect of supporting running of 32bit processes is the
ability to access 32bit syscalls. Such syscalls are invoked either by
using the legacy int 0x80 call gate interface or via the newer sysenter
instruction.

Ensure that if ia32 emulation is disabled (either at compile time or
runtime) then those 2 syscall mechanisms are also disabled.

Signed-off-by: Nikolay Borisov <[email protected]>
---
arch/x86/include/asm/proto.h | 3 +++
arch/x86/kernel/cpu/common.c | 37 ++++++++++++++++++------------------
arch/x86/kernel/idt.c | 10 ++++++++++
3 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
index 12ef86b19910..1241d27fc4a6 100644
--- a/arch/x86/include/asm/proto.h
+++ b/arch/x86/include/asm/proto.h
@@ -36,6 +36,9 @@ void entry_INT80_compat(void);
#ifdef CONFIG_XEN_PV
void xen_entry_INT80_compat(void);
#endif
+#else /* #CONFIG_IA32_EMULATION */
+#define entry_SYSCALL_compat NULL
+#define entry_SYSENTER_compat NULL
#endif

void x86_configure_nx(void);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b20774181e1a..aafb83d1b3a7 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -59,6 +59,7 @@
#include <asm/intel-family.h>
#include <asm/cpu_device_id.h>
#include <asm/uv/uv.h>
+#include <asm/ia32.h>
#include <asm/sigframe.h>
#include <asm/traps.h>
#include <asm/sev.h>
@@ -2053,24 +2054,24 @@ void syscall_init(void)
wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);

-#ifdef CONFIG_IA32_EMULATION
- wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
- /*
- * This only works on Intel CPUs.
- * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
- * This does not cause SYSENTER to jump to the wrong location, because
- * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
- */
- wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
- wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
- (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
- wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
-#else
- wrmsrl_cstar((unsigned long)entry_SYSCALL32_ignore);
- wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
- wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
- wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
-#endif
+ if (ia32_enabled()) {
+ wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
+ /*
+ * This only works on Intel CPUs.
+ * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
+ * This does not cause SYSENTER to jump to the wrong location, because
+ * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
+ */
+ wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
+ wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
+ (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
+ wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
+ } else {
+ wrmsrl_cstar((unsigned long)entry_SYSCALL32_ignore);
+ wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
+ wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
+ wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
+ }

/*
* Flags to clear on syscall; clear as much as possible
diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
index a58c6bc1cd68..c76953656212 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -10,6 +10,7 @@
#include <asm/proto.h>
#include <asm/desc.h>
#include <asm/hw_irq.h>
+#include <asm/ia32.h>
#include <asm/idtentry.h>

#define DPL0 0x0
@@ -116,6 +117,9 @@ static const __initconst struct idt_data def_idts[] = {
#endif

SYSG(X86_TRAP_OF, asm_exc_overflow),
+};
+
+static const struct idt_data ia32_idt[] __initconst = {
#if defined(CONFIG_IA32_EMULATION)
SYSG(IA32_SYSCALL_VECTOR, entry_INT80_compat),
#elif defined(CONFIG_X86_32)
@@ -226,6 +230,12 @@ void __init idt_setup_early_traps(void)
void __init idt_setup_traps(void)
{
idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
+
+ if (ia32_enabled()) {
+ idt_setup_from_table(idt_table, ia32_idt, ARRAY_SIZE(ia32_idt),
+ true);
+ }
+
}

#ifdef CONFIG_X86_64
--
2.34.1



2023-06-18 22:01:52

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] x86/entry: Make IA32 syscalls' availability depend on ia32_enabled()

On Fri, Jun 16 2023 at 15:57, Nikolay Borisov wrote:
> Another major aspect of supporting running of 32bit processes is the
> ability to access 32bit syscalls. Such syscalls are invoked either by
> using the legacy int 0x80 call gate interface or via the newer sysenter
> instruction.
>
> Ensure that if ia32 emulation is disabled (either at compile time or
> runtime) then those 2 syscall mechanisms are also disabled.

AFAICT there are _three_ mechanisms for 32bit syscalls, no?

> void __init idt_setup_traps(void)
> {
> idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
> +
> + if (ia32_enabled()) {
> + idt_setup_from_table(idt_table, ia32_idt, ARRAY_SIZE(ia32_idt),
> + true);

Just let it stick out. The 80 character limit is history.

Thanks,

tglx

2023-06-19 06:52:18

by Nikolay Borisov

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] x86/entry: Make IA32 syscalls' availability depend on ia32_enabled()



On 19.06.23 г. 0:17 ч., Thomas Gleixner wrote:
> On Fri, Jun 16 2023 at 15:57, Nikolay Borisov wrote:
>> Another major aspect of supporting running of 32bit processes is the
>> ability to access 32bit syscalls. Such syscalls are invoked either by
>> using the legacy int 0x80 call gate interface or via the newer sysenter
>> instruction.
>>
>> Ensure that if ia32 emulation is disabled (either at compile time or
>> runtime) then those 2 syscall mechanisms are also disabled.
>
> AFAICT there are _three_ mechanisms for 32bit syscalls, no?

int 0x80 and sysenter make it 2? Which one is the 3rd one - the "native
64bit syscall" used in for X32 ABI ? This patch specifically deals with
the first 2?
>
>> void __init idt_setup_traps(void)
>> {
>> idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
>> +
>> + if (ia32_enabled()) {
>> + idt_setup_from_table(idt_table, ia32_idt, ARRAY_SIZE(ia32_idt),
>> + true);
>
> Just let it stick out. The 80 character limit is history.
>
> Thanks,
>
> tglx

2023-06-19 08:50:47

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v3 5/5] x86/entry: Make IA32 syscalls' availability depend on ia32_enabled()

On Mon, Jun 19 2023 at 09:28, Nikolay Borisov wrote:
> On 19.06.23 г. 0:17 ч., Thomas Gleixner wrote:
>> On Fri, Jun 16 2023 at 15:57, Nikolay Borisov wrote:
>>> Another major aspect of supporting running of 32bit processes is the
>>> ability to access 32bit syscalls. Such syscalls are invoked either by
>>> using the legacy int 0x80 call gate interface or via the newer sysenter
>>> instruction.
>>>
>>> Ensure that if ia32 emulation is disabled (either at compile time or
>>> runtime) then those 2 syscall mechanisms are also disabled.
>>
>> AFAICT there are _three_ mechanisms for 32bit syscalls, no?
>
> int 0x80 and sysenter make it 2? Which one is the 3rd one - the "native
> 64bit syscall" used in for X32 ABI ? This patch specifically deals with
> the first 2?

int 80, sysenter, syscall = 3

They obviously depend on the vendor preference when the CPU has enabled
long mode:

AMD Intel
compat_int 80 y y
compat_sysenter #UD y
compat_syscall y #UD

On Intel SYSENTER is trivial to disable by setting MSR_IA32_SYSENTER_CS
to 0 which makes sysenter raise #GP.

The nasty one is SYSCALL on AMD. If MSR_EFER.SCE=1 then MSR_CSTAR must
contain a valid kernel text address because otherwise compat SYSCALL
faults with CPL0 and user GSBASE. That's the whole reason for the stub
function which just sets EAX to -ENOSYS and returns via SYSRET.

And your patch deals with all _three_:

compat=ON compat=OFF
compat_int 80: Set system interrupt gate ---

compat_sysenter: Set up SYSENTER MSRs for Invalidate SYSENTER
entry_SYSENTER_compat() MSRs

compat_syscall: Set MSR_CSTAR to Set MSR_CSTAR to
entry_SYSCALL_compat() stub function
(AMD only) (AMD only)

No?

Changelogs have to be precise. Otherwise they are useless and in the
worst case actively misleading.

Thanks,

tglx