arm64: Make Aarch32 compatibility enablement optional at boot

This is the second attempt of the patch, reviewed as follows:

* Reworked subject and description to avoid the term 'emulation' and to address generically 'exceptions' instead of 'syscalls' (mark.rutland)

* Moved aarch32_enabled() check inside system_supports_32bit_el0() (mark.rutland)

* Renamed AARCH32_EMULATION_DEFAULT_DISABLED to AARCH32_SUPPORT_DEFAULT_DISABLED (mark.rutland)

* Fixed a compilation Warning about missing function prototype

This is just for completeness since other possible solutions have been proposed that could be better suited, see for example:
https://lkml.kernel.org/linux-fsdevel/[email protected]/
and followups. So, this patchset is just for reference, may be useful in the future if some kind of exploit is found to bypass the 32bit process enablement check (letting a process call 32bit syscalls) and nothing better has been proposed meanwhile.

Andrea della Porta (4):
arm64: Introduce aarch32_enabled() arm64/process: Make loading of 32bit processes depend on

    aarch32_enabled()
  arm64/entry-common: Make Aarch32 exceptions' availability depend on
    aarch32_enabled()

arm64: Make Aarch32 support boot time configurable

 .../admin-guide/kernel-parameters.txt         |  7 ++++
 arch/arm64/Kconfig                            |  9 +++++
 arch/arm64/include/asm/cpufeature.h           | 20 +++++++++--
 arch/arm64/include/asm/exception.h            |  7 ++++
 arch/arm64/kernel/entry-common.c              | 33 +++++++++++++++++--
 5 files changed, 71 insertions(+), 5 deletions(-)

2.35.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Aarch32 bit support on 64bit kernels depends on whether CONFIG_COMPAT is selected or not. As it is a compile time option it doesn't provide the flexibility to have distributions set their own policy for Aarch32 support and give the user the flexibility to override it.

As a first step introduce aarch32_enabled() which abstracts whether 32 bit compat is turned on or off. Upcoming patches will implement the ability to set Aarch32 compat state at boot time.

---
 arch/arm64/include/asm/cpufeature.h | 15 +++++++++++++++
 arch/arm64/kernel/entry-common.c    |  2 ++
 2 files changed, 17 insertions(+)

index 396af9b9c857..1180d68daaff 100644
@@ -657,6 +657,21 @@ static inline bool supports_clearbhb(int scope)
 						    ID_AA64ISAR2_EL1_CLRBHB_SHIFT);
 }
 
+#ifdef CONFIG_COMPAT
+extern bool __aarch32_enabled;
+
+static inline bool aarch32_enabled(void)
+{
+	return __aarch32_enabled;
+}
+#else /* !CONFIG_COMPAT */
+
+static inline bool aarch32_enabled(void)
+{
+	return false;
+}
+#endif
+
 const struct cpumask *system_32bit_el0_cpumask(void);
 DECLARE_STATIC_KEY_FALSE(arm64_mismatched_32bit_el0);
 

index 0fc94207e69a..69ff9b8c0bde 100644
@@ -877,6 +877,8 @@ asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
 {
 	__el0_error_handler_common(regs);
 }
+
+bool __aarch32_enabled __ro_after_init = true;
 #else /* CONFIG_COMPAT */
 UNHANDLED(el0t, 32, sync)
 UNHANDLED(el0t, 32, irq)

2.35.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Other than patch #3, which as previously mentioned should be unnecessary if the kernel correctly never starts an "unsupported" AArch32 process to begin with, what does this do that simply overriding ID_AA64PFR0_EL1.EL0 via the existing idreg-override mechanism wouldn't?

Thanks,
Robin.

You're right, I guess we can simpluy leverage system_supports_32bit_el0() calling id_aa64pfr0_32bit_el0() and override the el0 nibble from command line, instead of inventing a new kernel parameter. For the sake of simplicity, maybe we can add a new alias in idreg-override, something like 'arm64.no32bit-el0'.

Thanks,
Andrea

Major aspect of Aarch32 support is the ability to load 32bit processes.
That's currently decided (among others) by compat_elf_check_arch() and system_supports_32bit_el0().

Make the macro use aarch32_enabled() to decide if Aarch32 compat is enabled before loading a 32bit process.

---
 arch/arm64/include/asm/cpufeature.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 1180d68daaff..778f2f3b2c7d 100644
@@ -679,8 +679,9 @@ static inline bool system_supports_32bit_el0(void)
 {
 	u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
 
-	return static_branch_unlikely(&arm64_mismatched_32bit_el0) ||
-	       id_aa64pfr0_32bit_el0(pfr0);
+	return (static_branch_unlikely(&arm64_mismatched_32bit_el0) ||
+		id_aa64pfr0_32bit_el0(pfr0)) &&
+		aarch32_enabled();
 }
 
 static inline bool system_supports_4kb_granule(void)

2.35.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Another major aspect of supporting running of 32bit processes is the ability to access 32bit syscalls and exceptions. Syscalls, in particular, can be invoked by using the svc instruction.

If Aarch32 support is disabled ensure that calling svc (or any exceptions) from 32bit context results in the same behavior as if CONFIG_COMPAT has not been enabled (i.e. a kernel panic).

---
 arch/arm64/include/asm/exception.h |  7 +++++++
 arch/arm64/kernel/entry-common.c   | 25 ++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

index ad688e157c9b..ccb41ba8d86c 100644
@@ -48,6 +48,13 @@ asmlinkage void el0t_32_irq_handler(struct pt_regs *regs);
 asmlinkage void el0t_32_fiq_handler(struct pt_regs *regs);
 asmlinkage void el0t_32_error_handler(struct pt_regs *regs);
 
+#ifdef CONFIG_COMPAT
+asmlinkage void el0t_32_sync_ni_handler(struct pt_regs *regs);
+asmlinkage void el0t_32_irq_ni_handler(struct pt_regs *regs);
+asmlinkage void el0t_32_fiq_ni_handler(struct pt_regs *regs);
+asmlinkage void el0t_32_error_ni_handler(struct pt_regs *regs);
+#endif
+
 asmlinkage void call_on_irq_stack(struct pt_regs *regs,
 				  void (*func)(struct pt_regs *));
 asmlinkage void asm_exit_to_user_mode(struct pt_regs *regs);

index 69ff9b8c0bde..32761760d9dd 100644
@@ -802,6 +802,11 @@ asmlinkage void noinstr el0t_64_error_handler(struct pt_regs *regs)
 }
 
 #ifdef CONFIG_COMPAT
+UNHANDLED(el0t, 32, sync_ni)
+UNHANDLED(el0t, 32, irq_ni)
+UNHANDLED(el0t, 32, fiq_ni)
+UNHANDLED(el0t, 32, error_ni)
+
 static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
 {
 	enter_from_user_mode(regs);

@@ -821,6 +826,11 @@ static void noinstr el0_svc_compat(struct pt_regs *regs)
 
 asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 {
+	if (!aarch32_enabled()) {
+		el0t_32_sync_ni_handler(regs);
+		return;
+	}
+
 	unsigned long esr = read_sysreg(esr_el1);
 
 	switch (ESR_ELx_EC(esr)) {

@@ -865,17 +875,26 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 
 asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
 {
-	__el0_irq_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_irq_ni_handler(regs);
+	else
+		__el0_irq_handler_common(regs);
 }
 
 asmlinkage void noinstr el0t_32_fiq_handler(struct pt_regs *regs)
 {
-	__el0_fiq_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_fiq_ni_handler(regs);
+	else
+		__el0_fiq_handler_common(regs);
 }
 
 asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
 {
-	__el0_error_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_error_ni_handler(regs);
+	else
+		__el0_error_handler_common(regs);
 }
 
 bool __aarch32_enabled __ro_after_init = true;

2.35.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Just to be clear, as it stands, I don't think we should apply this patch.

* There's no justficiation so far for disabling the vectors given they should be unreachable.

* If we really want to do this, the behaviour should be driven by a cpucap, so as to have truly negligible impact and to be consistent with how we handle features elsewhere.

That will require some preparatory rework to the way de handle detecing support for AArch32 at EL0 (some of which I think we should do anyway as a cleanup).

* We should not introduce new *_ni_handler() functions. If we really want to do this, we should refactor the existing code such that we have a single el0t_32_<vector>_handler() implementation for each vector regardless of CONFIG_COMPAT, which can figure out what to do, e.g. something like:

  | asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
  | {
  |         if (!system_suports_compat_el0())
  | 	            panic_unhandled_vector(el0t, 32, irq, regs);
  |             
  | 	    __el0_irq_handler_common(regs);
  | }

That way the feature check only needs to test IS_ENABLED(CONFIG_COMPAT) and alternative_has_cap(whatever), and we can rely on the compiler to elide the unreachable code. That way we use the exact same code for the case that 32-bit support is disabled statically or dynamically.

I had a quick go at mocking that up:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/entry/unhandled-rework

... which doesn't look too hard.

True, but let's see what it buys and what not:

- is it really necessary to check for 32 bit enablement when you cannot run 32 bit excecutable (from binfmt loader) in teh first place? Obviously not at all, unless you find a way (of which I'm not aware right now) to switch to 32 bit mode maybe by expliting some execpotion return path. In that (admittedly unlikely, as of now) case, you would expose all 32 bit syscall to be used by userspace.

- does 'redundantly' checking for 32 bit alignment in the vector handler pose some performence bottleneck? Yes, but only in the dynamic enabled case (i.e. you have enabled the 32 bit support via the proposed kernel parameter). All other cases are compiler optimized away or simply not reachable under normal circumstances, so it's redundant only in very few (and probably irrelevant) cases.

I agree with that, so assuming we still want to proceed and elaborate further, I'd like to sort the various proposals since I'm a little bit confused about
them: from one side you propose to rework the vector handler definitions and to insert the conditional logic inside them. On the other side there is the thread from you, Kees and others that eventually led to a seccomp assisted solution as the preferred way. I'm not entirely sure how seccomp can be used here, whether setting up a bpf filter from inside the kernel (that enable or disable the 32 bit handler based on the kernel parameter) or from userspace (systemd, others..), so I assume we're still interested to investigate on your proposed solution. Regarding that, I propose the following:

- rework the vector handler definitions as you've already mocked up, in order to get rid of the UNHANDLED declarations.

- use the actual system_supports_32bit_el0() call as the conditional inside the vector handler. That routine in turn is calling id_aa64pfr0_32bit_el0() that should load the ID_AA64PFR0_EL1. We can override the EL0 nibble by leveraging the idreg-override machinery, as noted by robin.murphy. In this way we don't even need to add a new kernel parameter, maybe just a user friendly mnemonic alias to that register, such like 'arm64.no32bit-el0'.

- CONFIG_COMPAT can be checked in the vector handler as you proposed in your mock up or maybe event inside system_supports_32bit_el0() - need to check whether the compiler is able to optimize away the static case even in that case.

Does it sounds reasonable?

Many thanks,
Andrea

Distributions would like to reduce their attack surface as much as possible but at the same time they'd want to retain flexibility to cater to a variety of legacy software. This stems from the conjecture that compat layer is likely rarely tested and could have latent security bugs. Ideally distributions will set their default policy and also give users the ability to override it as appropriate.

To enable this use case, introduce CONFIG_AARCH32_SUPPORT_DEFAULT_DISABLED compile time option, which controls whether 32bit processes/exceptions should be allowed or not. This option is aimed mainly at distributions to set their preferred default behavior in their kernels.

To allow users to override the distro's policy, introduce the 'allow_32bit_el0' parameter which allows overriding CONFIG_AARCH32_SUPPORT_DEFAULT_DISABLED state at boot time.

---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 arch/arm64/Kconfig                              | 9 +++++++++
 arch/arm64/kernel/entry-common.c                | 8 +++++++-
 3 files changed, 23 insertions(+), 1 deletion(-)

index 0a1731a0f0ef..9752c4640bd7 100644
@@ -1,3 +1,10 @@
+	allow_32bit_el0=	[ARM64]
+			Format: <bool>
+			When true, allows loading 32-bit programs and executing
+			32-bit syscalls and exceptions, essentially overriding
+			AARCH32_SUPPORT_DEFAULT_DISABLED at boot time. when false,
+			unconditionally disables AARCH32 support.
+
 	acpi=		[HW,ACPI,X86,ARM64,RISCV64]
 			Advanced Configuration and Power Interface
 			Format: { force | on | off | strict | noirq | rsdt |

index b10515c0200b..c8e1b3535018 100644
@@ -1725,6 +1725,15 @@ config SETEND_EMULATION
 	  If unsure, say Y
 endif # ARMV8_DEPRECATED
 
+config AARCH32_SUPPORT_DEFAULT_DISABLED
+	bool "Aarch32 support disabled by default"
+	default n
+	depends on COMPAT
+	help
+	  Make Aarch32 support disabled by default. This prevents loading 32-bit
+	  processes and access to 32-bit syscalls and exceptions.
+
+	  If unsure, leave it to its default value.
 endif # COMPAT
 
 menu "ARMv8.1 architectural features"

index 32761760d9dd..7698057ef4ce 100644
@@ -897,7 +897,13 @@ asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
 		__el0_error_handler_common(regs);
 }
 
-bool __aarch32_enabled __ro_after_init = true;
+bool __aarch32_enabled __ro_after_init = !IS_ENABLED(CONFIG_AARCH32_SUPPORT_DEFAULT_DISABLED);
+
+static int aarch32_support_override_cmdline(char *arg)
+{
+	return kstrtobool(arg, &__aarch32_enabled);
+}
+early_param("allow_32bit_el0", aarch32_support_override_cmdline);
 #else /* CONFIG_COMPAT */
 UNHANDLED(el0t, 32, sync)
 UNHANDLED(el0t, 32, irq)

2.35.3


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel