* [PATCH v2 0/4] Make IA32_EMULATION boot time overridable
@ 2023-06-09 11:13 Nikolay Borisov
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 11:13 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
Here's the 2nd version of the patch which aims to make IA32_EMULATION essentially
a boot time option. The changes in this posting are:
* Introduced a compile-time option CONFIG_IA32_EMULATION_DEFAULT_DISABLED which
can be set during compile time and can be overriden at boot time via a new
parameter 'ia32_mode'.
* Documented the new parameter as per Thomas' suggestion
* Added a new patch which renames ignore_sysret as per Andrew Cooper's suggestion
* Fixed compat_elf_check_arch condition check to only affect compat process
loading and leave x32 abi processes alone
* Dropped GDT modification as this was deemed a separate change. Likely I'd need
to follow up with a more complete solution.
Nikolay Borisov (4):
x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option
x86/entry: Rename ignore_sysret and compile it unconditionally
x86/entry: Disable IA32 syscall if ia32_disabled is true
x86: Disable laoding 32bit processes if ia32_disabled is true
.../admin-guide/kernel-parameters.txt | 4 ++
arch/x86/Kconfig | 5 +++
arch/x86/entry/common.c | 16 ++++++++
arch/x86/entry/entry_64.S | 6 +--
arch/x86/include/asm/desc.h | 1 +
arch/x86/include/asm/elf.h | 9 ++++-
arch/x86/include/asm/processor.h | 2 +-
arch/x86/include/asm/traps.h | 4 ++
arch/x86/kernel/cpu/common.c | 37 ++++++++++---------
arch/x86/kernel/idt.c | 7 ++++
10 files changed, 67 insertions(+), 24 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
@ 2023-06-09 11:13 ` Nikolay Borisov
2023-06-09 15:06 ` Thomas Gleixner
2023-06-10 2:21 ` Randy Dunlap
2023-06-09 11:13 ` [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally Nikolay Borisov
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 11:13 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
Distributions would like to reduce their attack surface as much as
possible but at the same time they have to cater to a wide variety of
legacy software. One such avenue where distros have to strike a balance
is the support for 32bit syscalls on a 64bit kernel. Ideally
distributions would have a way to set that policy in their kernel config
files and at the same time users should also have the ability to
override this decision. Introduce such mechanism in the face of
CONFIG_IA32_EMULATION_DEFAULT_DISABLED compile time option, which
defaults to 'N' i.e retains current behavio in case
CONFIG_IA32_EMULATION is enabled. If, however, a distributor would like
to change this policy they can do so via the newly introduced
CONFIG_IA32_EMULATION_DEFAULT_DISABLED. As a final note allow users to
override the decision via the ia32_mode boot time parameter.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
Documentation/admin-guide/kernel-parameters.txt | 4 ++++
arch/x86/Kconfig | 5 +++++
arch/x86/entry/common.c | 16 ++++++++++++++++
arch/x86/include/asm/traps.h | 4 ++++
4 files changed, 29 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9e5bab29685f..7c01ab8bcd56 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1865,6 +1865,10 @@
0 -- machine default
1 -- force brightness inversion
+ ia32_mode= [X86-64]
+ Format: ia32_mode=disabled, ia32_mode=enabled
+ Allows to override the compile-time IA32_EMULATION option at boot time
+
icn= [HW,ISDN]
Format: <io>[,<membase>[,<icn_id>[,<icn_id2>]]]
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 53bab123a8ee..9c32fd720701 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -3038,6 +3038,11 @@ config IA32_EMULATION
64-bit kernel. You should likely turn this on, unless you're
100% sure that you don't have any 32-bit programs left.
+config IA32_EMULATION_DEFAULT_DISABLED
+ bool "IA32 Emulation default disabled"
+ default n
+ depends on IA32_EMULATION
+
config X86_X32_ABI
bool "x32 ABI for 64-bit mode"
depends on X86_64
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6c2826417b33..6da89575e03e 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -19,6 +19,7 @@
#include <linux/nospec.h>
#include <linux/syscalls.h>
#include <linux/uaccess.h>
+#include <linux/init.h>
#ifdef CONFIG_XEN_PV
#include <xen/xen-ops.h>
@@ -96,6 +97,21 @@ static __always_inline int syscall_32_enter(struct pt_regs *regs)
return (int)regs->orig_ax;
}
+#ifdef CONFIG_IA32_EMULATION
+bool ia32_disabled = IS_ENABLED(CONFIG_IA32_EMULATION_DEFAULT_DISABLED);
+
+static int ia32_mode_override_cmdline(char *arg)
+{
+ if (!strcmp(arg, "disabled"))
+ ia32_disabled = true;
+ else if (!strcmp(arg, "enabled"))
+ ia32_disabled = false;
+
+ return 1;
+}
+__setup("ia32_mode=", ia32_mode_override_cmdline);
+#endif
+
/*
* Invoke a 32-bit syscall. Called with IRQs on in CONTEXT_KERNEL.
*/
diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
index 47ecfff2c83d..dd93aac3718b 100644
--- a/arch/x86/include/asm/traps.h
+++ b/arch/x86/include/asm/traps.h
@@ -20,6 +20,10 @@ asmlinkage __visible noinstr struct pt_regs *vc_switch_off_ist(struct pt_regs *e
extern bool ibt_selftest(void);
+#ifdef CONFIG_IA32_EMULATION
+extern bool ia32_disabled;
+#endif
+
#ifdef CONFIG_X86_F00F_BUG
/* For handling the FOOF bug */
void handle_invalid_op(struct pt_regs *regs);
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
@ 2023-06-09 11:13 ` Nikolay Borisov
2023-06-09 15:08 ` Thomas Gleixner
2023-06-09 11:13 ` [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true Nikolay Borisov
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 11:13 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
Give ignore_sysret a more descriptive name as it's actually used to make
32bit syscalls a noop and return ENOSYS, rather than doing anything
special to sysret. While at it also compile the function unconditinally
as this is going to be used in the patch disabling ia32 syscalls due to
'ia32_disabled' parameter.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/entry/entry_64.S | 6 ++----
arch/x86/include/asm/processor.h | 2 +-
arch/x86/kernel/cpu/common.c | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f31e286c2977..7068af44008a 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1514,18 +1514,16 @@ SYM_CODE_START(asm_exc_nmi)
iretq
SYM_CODE_END(asm_exc_nmi)
-#ifndef CONFIG_IA32_EMULATION
/*
* This handles SYSCALL from 32-bit code. There is no way to program
* MSRs to fully disable 32-bit SYSCALL.
*/
-SYM_CODE_START(ignore_sysret)
+SYM_CODE_START(entry_SYSCALL32_ignore)
UNWIND_HINT_END_OF_STACK
ENDBR
mov $-ENOSYS, %eax
sysretl
-SYM_CODE_END(ignore_sysret)
-#endif
+SYM_CODE_END(entry_SYSCALL32_ignore)
.pushsection .text, "ax"
__FUNC_ALIGN
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a1e4fa58b357..61c10b4e3e35 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -399,7 +399,7 @@ static inline unsigned long cpu_kernelmode_gs_base(int cpu)
return (unsigned long)per_cpu(fixed_percpu_data.gs_base, cpu);
}
-extern asmlinkage void ignore_sysret(void);
+extern asmlinkage void entry_SYSCALL32_ignore(void);
/* Save actual FS/GS selectors and bases to current->thread */
void current_save_fsgs(void);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 80710a68ef7d..b20774181e1a 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2066,7 +2066,7 @@ void syscall_init(void)
(unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
#else
- wrmsrl_cstar((unsigned long)ignore_sysret);
+ 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);
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
2023-06-09 11:13 ` [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally Nikolay Borisov
@ 2023-06-09 11:13 ` Nikolay Borisov
2023-06-09 15:22 ` Thomas Gleixner
2023-06-09 11:13 ` [PATCH v2 4/4] x86: Disable laoding 32bit processes " Nikolay Borisov
2023-06-10 21:46 ` [PATCH v2 0/4] Make IA32_EMULATION boot time overridable David Laight
4 siblings, 1 reply; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 11:13 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
First stage of disabling ia32 compat layer is to disable 32bit syscall
entry points. Legacy int 0x80 vector is disabled by zeroing out its gate
descriptor in the idt and the sysenter vector is disabled by re-using
the existing code in case IA32_EMULATION is disabled.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/include/asm/desc.h | 1 +
arch/x86/kernel/cpu/common.c | 37 ++++++++++++++++++------------------
arch/x86/kernel/idt.c | 7 +++++++
3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index ab97b22ac04a..1182a5b10be9 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -8,6 +8,7 @@
#include <asm/fixmap.h>
#include <asm/irq_vectors.h>
#include <asm/cpu_entry_area.h>
+#include <asm/traps.h>
#include <linux/debug_locks.h>
#include <linux/smp.h>
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b20774181e1a..3c4055184d0f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2053,24 +2053,25 @@ 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 ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
+ !IS_ENABLED(CONFIG_IA32_EMULATION)) {
+ 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);
+ } else {
+ 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);
+ }
/*
* 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..d1f388ef2e66 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -226,6 +226,13 @@ 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 (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
+ gate_desc null_desc = {};
+ write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc);
+ clear_bit(IA32_SYSCALL_VECTOR, system_vectors);
+ }
+
}
#ifdef CONFIG_X86_64
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
` (2 preceding siblings ...)
2023-06-09 11:13 ` [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true Nikolay Borisov
@ 2023-06-09 11:13 ` Nikolay Borisov
2023-06-09 15:26 ` Thomas Gleixner
2023-06-09 16:45 ` Brian Gerst
2023-06-10 21:46 ` [PATCH v2 0/4] Make IA32_EMULATION boot time overridable David Laight
4 siblings, 2 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 11:13 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
In addition to disabling 32bit syscall interface let's also disable the
ability to load 32bit compat processes.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/include/asm/elf.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 18fd06f7936a..0fa49388ff16 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -148,9 +148,16 @@ do { \
#define elf_check_arch(x) \
((x)->e_machine == EM_X86_64)
+#ifdef CONFIG_IA32_EMULATION
+extern bool ia32_disabled;
#define compat_elf_check_arch(x) \
- (elf_check_arch_ia32(x) || \
+ ((elf_check_arch_ia32(x) && !ia32_disabled) || \
(IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64))
+#else
+#define compat_elf_check_arch(x) \
+ (elf_check_arch_ia32(x) || \
+ (IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64))
+#endif
static inline void elf_common_init(struct thread_struct *t,
struct pt_regs *regs, const u16 ds)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
@ 2023-06-09 15:06 ` Thomas Gleixner
2023-06-10 2:21 ` Randy Dunlap
1 sibling, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2023-06-09 15:06 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> Distributions would like to reduce their attack surface as much as
> possible but at the same time they have to cater to a wide variety of
> legacy software. One such avenue where distros have to strike a balance
> is the support for 32bit syscalls on a 64bit kernel. Ideally
> distributions would have a way to set that policy in their kernel config
> files and at the same time users should also have the ability to
> override this decision. Introduce such mechanism in the face of
> CONFIG_IA32_EMULATION_DEFAULT_DISABLED compile time option, which
> defaults to 'N' i.e retains current behavio in case
> CONFIG_IA32_EMULATION is enabled. If, however, a distributor would like
> to change this policy they can do so via the newly introduced
> CONFIG_IA32_EMULATION_DEFAULT_DISABLED. As a final note allow users to
> override the decision via the ia32_mode boot time parameter.
Can you please stop rushing stuff out instead of sitting down first and
reading the documentation others have put together for reasons?
I gave you the pointers and I'm refusing to read and decode the
above. Here it is again:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
It's not that hard, really.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally
2023-06-09 11:13 ` [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally Nikolay Borisov
@ 2023-06-09 15:08 ` Thomas Gleixner
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2023-06-09 15:08 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> Give ignore_sysret a more descriptive name as it's actually used to make
> 32bit syscalls a noop and return ENOSYS, rather than doing anything
> special to sysret. While at it also compile the function unconditinally
> as this is going to be used in the patch disabling ia32 syscalls due to
> 'ia32_disabled' parameter.
ONE patch does ONE thing. Rename is ONE thing. Make it undonditional is
ANOTHER thing. They are independent and especially the second one is
not 'while at it'.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true
2023-06-09 11:13 ` [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true Nikolay Borisov
@ 2023-06-09 15:22 ` Thomas Gleixner
2023-06-09 16:03 ` Nikolay Borisov
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2023-06-09 15:22 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> First stage of disabling ia32 compat layer is to disable 32bit syscall
> entry points. Legacy int 0x80 vector is disabled by zeroing out its gate
> descriptor in the idt and the sysenter vector is disabled by re-using
> the existing code in case IA32_EMULATION is disabled.
This describes WHAT the patch does without providing any context.
> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
> + !IS_ENABLED(CONFIG_IA32_EMULATION)) {
I told you before that my brain based compiler complains about your
patches not building with CONFIG_IA32_EMULATION=n. The above still fails
to build.
Aside of that this condition is convoluted and can be simplified to
exactly a simple and understandable
if (foo)
which is actually the obvious solution to make it compile in all
configurations.
It's not too much asked to flip the config switch which affects the code
you are changing for a test.
> @@ -226,6 +226,13 @@ 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 (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
Ditto.
> + gate_desc null_desc = {};
Lacks a newline between declaration and code. It's documented to be
required, no?
> + write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc);
> + clear_bit(IA32_SYSCALL_VECTOR, system_vectors);
> + }
That aside, I asked you to split IA32_SYSCALL_VECTOR out of def_idts[]
and handle it separately, no? If you disagree with me then reply to my
review first instead of ignoring me silently.
I don't care about you wasting your time, but I very much care about you
wasting my time.
Stop this right now before it becomes a habit.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true
2023-06-09 11:13 ` [PATCH v2 4/4] x86: Disable laoding 32bit processes " Nikolay Borisov
@ 2023-06-09 15:26 ` Thomas Gleixner
2023-06-09 16:45 ` Brian Gerst
1 sibling, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2023-06-09 15:26 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby, Nikolay Borisov
On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 18fd06f7936a..0fa49388ff16 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -148,9 +148,16 @@ do { \
> #define elf_check_arch(x) \
> ((x)->e_machine == EM_X86_64)
>
> +#ifdef CONFIG_IA32_EMULATION
> +extern bool ia32_disabled;
> #define compat_elf_check_arch(x) \
1) Your keyboard clearly has a broken newline key.
2) Why is there still a duplicated declaration?
3) This #ifdeffery is not needed if this is done right.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true
2023-06-09 15:22 ` Thomas Gleixner
@ 2023-06-09 16:03 ` Nikolay Borisov
2023-06-09 16:13 ` Nikolay Borisov
2023-06-10 11:26 ` Thomas Gleixner
0 siblings, 2 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 16:03 UTC (permalink / raw)
To: Thomas Gleixner, x86; +Cc: linux-kernel, mhocko, jslaby
On 9.06.23 г. 18:22 ч., Thomas Gleixner wrote:
> On Fri, Jun 09 2023 at 14:13, Nikolay Borisov wrote:
>> First stage of disabling ia32 compat layer is to disable 32bit syscall
>> entry points. Legacy int 0x80 vector is disabled by zeroing out its gate
>> descriptor in the idt and the sysenter vector is disabled by re-using
>> the existing code in case IA32_EMULATION is disabled.
>
> This describes WHAT the patch does without providing any context.
>
>> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
>> + !IS_ENABLED(CONFIG_IA32_EMULATION)) {
>
> I told you before that my brain based compiler complains about your
> patches not building with CONFIG_IA32_EMULATION=n. The above still fails
> to build.
Yes, it does. My bad.
>
> Aside of that this condition is convoluted and can be simplified to
> exactly a simple and understandable
>
> if (foo)
>
> which is actually the obvious solution to make it compile in all
> configurations.
I fail to see how this can be done the way you suggest given that
ia32_disabled is visible iff IA32_EMULATION is selected, this means an
#ifdef is mandatory so that ia32_disabled is checked when we know it
will exist as a symbol, the same applies for the entry_SYSCALL_compat
symbol which has to be used iff IA32_EMULATION is defined. I.e the
ignore code should also be duplicated in the #ifdef IA32_EMULATION &&
ia32_disabled and in the #else condition.
>
> It's not too much asked to flip the config switch which affects the code
> you are changing for a test.
Sorry, missed it the first time.
>
>> @@ -226,6 +226,13 @@ 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 (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
>
> Ditto.
This actually doesn't fail, because if IA32_EMULATION is n that
conditional expands to 'if (0 && ia32_disabled)' which is eliminated by
the compiler.
>
>> + gate_desc null_desc = {};
>
> Lacks a newline between declaration and code. It's documented to be
> required, no?
>
>> + write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc);
>> + clear_bit(IA32_SYSCALL_VECTOR, system_vectors);
>> + }
>
> That aside, I asked you to split IA32_SYSCALL_VECTOR out of def_idts[]
> and handle it separately, no? If you disagree with me then reply to my
> review first instead of ignoring me silently.
I tried doing this but it's no go since def_its is defined statically.
Since tha IA32_SYSCALL_VECTOR is the last one it can't simply be tacked
at the end of this array in a separate place. Hence the only viable
solution ( apart from making def_its a dynamically sized array) was to
simply overwrite IA32_SYSCALL_VECTOR in idt_table before it's being
loaded into the ldtr.
<snip>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true
2023-06-09 16:03 ` Nikolay Borisov
@ 2023-06-09 16:13 ` Nikolay Borisov
2023-06-10 11:26 ` Thomas Gleixner
1 sibling, 0 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-09 16:13 UTC (permalink / raw)
To: Thomas Gleixner, x86; +Cc: linux-kernel, mhocko, jslaby
On 9.06.23 г. 19:03 ч., Nikolay Borisov wrote:
>
<snip>
>
>>
>>> @@ -226,6 +226,13 @@ 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 (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
>>
>> Ditto.
>
> This actually doesn't fail, because if IA32_EMULATION is n that
> conditional expands to 'if (0 && ia32_disabled)' which is eliminated by
> the compiler.
Forget this one, it works because I made ia32_disabled always compiled,
hoowever, the problem in syscall_init() remains because now
entry_SYSCALL_compat is also compiled in iff CONFIG_IA32_EMULATION is
selected.
<snip>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true
2023-06-09 11:13 ` [PATCH v2 4/4] x86: Disable laoding 32bit processes " Nikolay Borisov
2023-06-09 15:26 ` Thomas Gleixner
@ 2023-06-09 16:45 ` Brian Gerst
1 sibling, 0 replies; 16+ messages in thread
From: Brian Gerst @ 2023-06-09 16:45 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: x86, linux-kernel, mhocko, jslaby
On Fri, Jun 9, 2023 at 8:07 AM Nikolay Borisov <nik.borisov@suse.com> wrote:
>
> In addition to disabling 32bit syscall interface let's also disable the
> ability to load 32bit compat processes.
>
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
This patch needs to come beforethe patch that disables 32-bit
syscalls. It makes no sense to be able to load a 32-bit executable
that cannot make syscalls.
Brian Gerst
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
2023-06-09 15:06 ` Thomas Gleixner
@ 2023-06-10 2:21 ` Randy Dunlap
1 sibling, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2023-06-10 2:21 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby
On 6/9/23 04:13, Nikolay Borisov wrote:
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 53bab123a8ee..9c32fd720701 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -3038,6 +3038,11 @@ config IA32_EMULATION
> 64-bit kernel. You should likely turn this on, unless you're
> 100% sure that you don't have any 32-bit programs left.
>
> +config IA32_EMULATION_DEFAULT_DISABLED
> + bool "IA32 Emulation default disabled"
> + default n
> + depends on IA32_EMULATION
> +
I expected checkpatch to complain about no help text here.
Did it not complain?
--
~Randy
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true
2023-06-09 16:03 ` Nikolay Borisov
2023-06-09 16:13 ` Nikolay Borisov
@ 2023-06-10 11:26 ` Thomas Gleixner
1 sibling, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2023-06-10 11:26 UTC (permalink / raw)
To: Nikolay Borisov, x86; +Cc: linux-kernel, mhocko, jslaby
On Fri, Jun 09 2023 at 19:03, Nikolay Borisov wrote:
> On 9.06.23 г. 18:22 ч., Thomas Gleixner wrote:
>>> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
>>> + !IS_ENABLED(CONFIG_IA32_EMULATION)) {
>>
>> Aside of that this condition is convoluted and can be simplified to
>> exactly a simple and understandable
>>
>> if (foo)
>>
>> which is actually the obvious solution to make it compile in all
>> configurations.
>
> I fail to see how this can be done the way you suggest given that
> ia32_disabled is visible iff IA32_EMULATION is selected, this means an
> #ifdef is mandatory so that ia32_disabled is checked when we know it
> will exist as a symbol, the same applies for the entry_SYSCALL_compat
> symbol which has to be used iff IA32_EMULATION is defined. I.e the
> ignore code should also be duplicated in the #ifdef IA32_EMULATION &&
> ia32_disabled and in the #else condition.
That's wrong in every aspect. Neither the #ifdeffery nor the code
duplication is mandatory.
arch/x86/include/asm/XXXX.h
#ifdef CONFIG_IA32_EMULATION
extern bool __ia32_enabled;
static inline bool ia32_enabled(void)
{
return __ia32_enabled;
}
#else
static inline bool ia32_enabled(void)
{
return IS_ENABLED(CONFIG_X86_32);
}
#endif
if (ia32_enabled()) {
...
} else {
...
}
Which avoids the #ifdeffery in the code _and_ the code duplication.
All it needs aside of the above is to make sure that the other two
things which the compiler complains about being undeclared in the
EMULATION=n case are treated differently in the relevant header
file. It's not rocket science. See also below.
If you chose $XXXX.h carefully it simply works for everything including
asm/elf.h.
I surely wouldn't have suggested it if it wouldn't be feasible and
reasonably trivial. I wanted you to figure it out yourself instead of
serving you the solution on a silver tablet. There are tons of examples
in the code.
>>> @@ -226,6 +226,13 @@ 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 (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) {
>>
>> Ditto.
>
> This actually doesn't fail, because if IA32_EMULATION is n that
> conditional expands to 'if (0 && ia32_disabled)' which is eliminated by
> the compiler.
Making uninformed claims does not make it more correct.
This _cannot_ compile because the dead code elimination pass is not even
reached due to ia32_disabled being undeclared.
declaration != definition
and
#ifdef CONSTANT != if (CONSTANT)
Compiler basics.
You could have spared yourself the embarrassment and the lecture by
compiling that file with IA32_EMULATION=n or by carefully analysing the
compile fail of cpu/common.c. That code is not any different:
>>> + if ((IS_ENABLED(CONFIG_IA32_EMULATION) && ia32_disabled) ||
>>> + !IS_ENABLED(CONFIG_IA32_EMULATION)) {
and the compiler also complains rightfully about ia32_disabled being
undeclared _before_ complaining about entry_*_compat.
So:
// Declaration
extern bool foo;
if (0 && foo)
....
compiles and links without ever defining 'foo'.
While:
#if 0
extern bool foo;
#endif
if (0 && foo)
....
already fails to compile due to -Werror
See?
>>> + gate_desc null_desc = {};
>>
>> Lacks a newline between declaration and code. It's documented to be
>> required, no?
>>
>>> + write_idt_entry(idt_table, IA32_SYSCALL_VECTOR, &null_desc);
>>> + clear_bit(IA32_SYSCALL_VECTOR, system_vectors);
>>> + }
>>
>> That aside, I asked you to split IA32_SYSCALL_VECTOR out of def_idts[]
>> and handle it separately, no? If you disagree with me then reply to my
>> review first instead of ignoring me silently.
>
> I tried doing this but it's no go since def_its is defined statically.
> Since tha IA32_SYSCALL_VECTOR is the last one it can't simply be tacked
> at the end of this array in a separate place. Hence the only viable
> solution ( apart from making def_its a dynamically sized array) was to
> simply overwrite IA32_SYSCALL_VECTOR in idt_table before it's being
> loaded into the ldtr.
Obviously we have fundamentally different interpretations of the phrase
'split IA32_SYSCALL_VECTOR out of def_idts[] and handle it separately':
This splits it out:
def_idts[] = {
...
-#if defined(CONFIG_IA32_EMULATION)
- SYSG(IA32_SYSCALL_VECTOR, entry_INT80_compat),
-#elif defined(CONFIG_X86_32)
- SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32),
-#endif
};
+ia32_idt[] = {
+#if defined(CONFIG_IA32_EMULATION)
+ SYSG(IA32_SYSCALL_VECTOR, entry_INT80_compat),
+#elif defined(CONFIG_X86_32)
+ SYSG(IA32_SYSCALL_VECTOR, entry_INT80_32),
+#endif
+};
This handles it separately:
idt_setup_from_table(idt_table, def_idts, ARRAY_SIZE(def_idts), true);
+ if (ia32_enabled())
+ idt_setup_from_table(idt_table, ia32_idt,....);
Which makes it bloody obvious what this is about.
Are you still convinced that the only viable solution is clearing it
after the fact?
So please go and ensure that all config combinations work and that it
builds and works for 32bit too. The latter fails to build because of
including traps.h into an header for no reason.
There is absolutely no urgency to get this into the tree, so please take
your time and do not rush out the next half baken version of this,
unless you aim for the fast path to my ignore list.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2 0/4] Make IA32_EMULATION boot time overridable
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
` (3 preceding siblings ...)
2023-06-09 11:13 ` [PATCH v2 4/4] x86: Disable laoding 32bit processes " Nikolay Borisov
@ 2023-06-10 21:46 ` David Laight
2023-06-11 8:19 ` Nikolay Borisov
4 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2023-06-10 21:46 UTC (permalink / raw)
To: 'Nikolay Borisov', x86@kernel.org
Cc: linux-kernel@vger.kernel.org, mhocko@suse.com, jslaby@suse.cz
From: Nikolay Borisov
> Sent: 09 June 2023 12:13
>
> Here's the 2nd version of the patch which aims to make IA32_EMULATION essentially
> a boot time option. The changes in this posting are:
Does it make any sense to be able to enable/disable it at run-time
(through a sysctl).
Perhaps only if enabled at boot - where it can be a 'soft disable'
even though the cpu is initialised to allow the 32bit system calls.
Remember, if you are root (and the system isn't hard locked down)
it is pretty easy to change a global boolean variable.
So it doesn't really affect the attack surface.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/4] Make IA32_EMULATION boot time overridable
2023-06-10 21:46 ` [PATCH v2 0/4] Make IA32_EMULATION boot time overridable David Laight
@ 2023-06-11 8:19 ` Nikolay Borisov
0 siblings, 0 replies; 16+ messages in thread
From: Nikolay Borisov @ 2023-06-11 8:19 UTC (permalink / raw)
To: David Laight, x86@kernel.org
Cc: linux-kernel@vger.kernel.org, mhocko@suse.com, jslaby@suse.cz
On 11.06.23 г. 0:46 ч., David Laight wrote:
> From: Nikolay Borisov
>> Sent: 09 June 2023 12:13
>>
>> Here's the 2nd version of the patch which aims to make IA32_EMULATION essentially
>> a boot time option. The changes in this posting are:
>
> Does it make any sense to be able to enable/disable it at run-time
> (through a sysctl).
I'd say now, because then we are losing consistency, that is if the
sysctl is off, then some 32bit process is being run, then you flip it
back to on and suddenly this process dies (if it's using syscalls that is).
With a boot time switch we'll ensure that no 32bit process can be
loaded, which at least can give the sysadmin some assurance that the
machine is 32bit clean. Am I missing something?
> Perhaps only if enabled at boot - where it can be a 'soft disable'
> even though the cpu is initialised to allow the 32bit system calls.
>
> Remember, if you are root (and the system isn't hard locked down)
Well, if you are root then every bet's off?
> it is pretty easy to change a global boolean variable.
> So it doesn't really affect the attack surface.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-06-11 8:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-09 11:13 [PATCH v2 0/4] Make IA32_EMULATION boot time overridable Nikolay Borisov
2023-06-09 11:13 ` [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option Nikolay Borisov
2023-06-09 15:06 ` Thomas Gleixner
2023-06-10 2:21 ` Randy Dunlap
2023-06-09 11:13 ` [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally Nikolay Borisov
2023-06-09 15:08 ` Thomas Gleixner
2023-06-09 11:13 ` [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true Nikolay Borisov
2023-06-09 15:22 ` Thomas Gleixner
2023-06-09 16:03 ` Nikolay Borisov
2023-06-09 16:13 ` Nikolay Borisov
2023-06-10 11:26 ` Thomas Gleixner
2023-06-09 11:13 ` [PATCH v2 4/4] x86: Disable laoding 32bit processes " Nikolay Borisov
2023-06-09 15:26 ` Thomas Gleixner
2023-06-09 16:45 ` Brian Gerst
2023-06-10 21:46 ` [PATCH v2 0/4] Make IA32_EMULATION boot time overridable David Laight
2023-06-11 8:19 ` Nikolay Borisov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).