[PATCH v2 0/4] Make IA32_EMULATION boot time overridable

[PATCH v2 0/4] Make IA32_EMULATION boot time overridable

[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

[PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option

[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

Re: [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option

[Thomas Gleixner]

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

Re: [PATCH v2 1/4] x86: Introduce CONFIG_IA32_EMULATION_DEFAULT_DISABLED Kconfig option

[Randy Dunlap]

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

[PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally

[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

Re: [PATCH v2 2/4] x86/entry: Rename ignore_sysret and compile it unconditionally

[Thomas Gleixner]

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

[PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[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

Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[Thomas Gleixner]

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

Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[Nikolay Borisov]

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>

Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[Nikolay Borisov]

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>

Re: [PATCH v2 3/4] x86/entry: Disable IA32 syscall if ia32_disabled is true

[Thomas Gleixner]

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

[PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true

[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

Re: [PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true

[Thomas Gleixner]

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

Re: [PATCH v2 4/4] x86: Disable laoding 32bit processes if ia32_disabled is true

[Brian Gerst]

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

RE: [PATCH v2 0/4] Make IA32_EMULATION boot time overridable

[David Laight]

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)

Re: [PATCH v2 0/4] Make IA32_EMULATION boot time overridable

[Nikolay Borisov]

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