[PATCH] module/ksymtab: use 64-bit relative reference for target symbol

* [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
@ 2019-05-22 15:02 ` Ard Biesheuvel
  0 siblings, 0 replies; 19+ messages in thread
From: Ard Biesheuvel @ 2019-05-22 15:02 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: marc.zyngier, james.morse, will.deacon, guillaume.gardet,
	mark.rutland, mingo, jeyu, linux-kernel, linux-arch, arnd, x86,
	Ard Biesheuvel

The following commit

  7290d5809571 ("module: use relative references for __ksymtab entries")

updated the ksymtab handling of some KASLR capable architectures
so that ksymtab entries are emitted as pairs of 32-bit relative
references. This reduces the size of the entries, but more
importantly, it gets rid of statically assigned absolute
addresses, which require fixing up at boot time if the kernel
is self relocating (which takes a 24 byte RELA entry for each
member of the ksymtab struct).

Since ksymtab entries are always part of the same module as the
symbol they export (or of the core kernel), it was assumed at the
time that a 32-bit relative reference is always sufficient to
capture the offset between a ksymtab entry and its target symbol.

Unfortunately, this is not always true: in the case of per-CPU
variables, a per-CPU variable's base address (which usually differs
from the actual address of any of its per-CPU copies) could be at
an arbitrary offset from the ksymtab entry, and so it may be out
of range for a 32-bit relative reference.

To make matters worse, we identified an issue in the arm64 module
loader, where the overflow check applied to 32-bit place relative
relocations uses the range that is specified in the AArch64 psABI,
which is documented as having a 'blind spot' unless you explicitly
narrow the range to match the signed vs unsigned interpretation of
the relocation target [0]. This means that, in some cases, code
importing those per-CPU variables from other modules may obtain a
bogus reference and corrupt unrelated data.

So let's fix this issue by switching to a 64-bit place relative
reference on 64-bit architectures for the ksymtab entry's target
symbol. This uses a bit more memory in the entry itself, which is
unfortunate, but it preserves the original intent, which was to
make the value invariant under runtime relocation of the core
kernel.

[0] https://lore.kernel.org/linux-arm-kernel/20190521125707.6115-1-ard.biesheuvel@arm.com

Cc: Jessica Yu <jeyu@kernel.org>
Cc: <stable@vger.kernel.org> # v4.19+
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

Note that the name 'CONFIG_HAVE_ARCH_PREL32_RELOCATIONS' is no longer
entirely accurate after this patch, so I will follow up with a patch
to rename it to CONFIG_HAVE_ARCH_PREL_RELOCATIONS, but that doesn't
require a backport to -stable so I have omitted it here.

Also note that for x86, this patch depends on b40a142b12b5 ("x86: Add
support for 64-bit place relative relocations"), which will need to
be backported to v4.19 (from v4.20) if this patch is applied to
-stable.

 include/asm-generic/export.h |  9 +++++++--
 include/linux/compiler.h     |  9 +++++++++
 include/linux/export.h       | 14 ++++++++++----
 kernel/module.c              |  2 +-
 4 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 294d6ae785d4..4d658b1e4707 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -4,7 +4,7 @@
 #ifndef KSYM_FUNC
 #define KSYM_FUNC(x) x
 #endif
-#ifdef CONFIG_64BIT
+#if defined(CONFIG_64BIT) && !defined(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)
 #ifndef KSYM_ALIGN
 #define KSYM_ALIGN 8
 #endif
@@ -19,7 +19,12 @@
 
 .macro __put, val, name
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-	.long	\val - ., \name - .
+#ifdef CONFIG_64BIT
+	.quad	\val - .
+#else
+	.long	\val - .
+#endif
+	.long	\name - .
 #elif defined(CONFIG_64BIT)
 	.quad	\val, \name
 #else
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b0..33c65ebb7cfe 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -305,6 +305,15 @@ static inline void *offset_to_ptr(const int *off)
 	return (void *)((unsigned long)off + *off);
 }
 
+/**
+ * loffset_to_ptr - convert a relative memory offset to an absolute pointer
+ * @off:	the address of the signed long offset value
+ */
+static inline void *loffset_to_ptr(const long *off)
+{
+	return (void *)((unsigned long)off + *off);
+}
+
 #endif /* __ASSEMBLY__ */
 
 /* Compile time object size, -1 for unknown */
diff --git a/include/linux/export.h b/include/linux/export.h
index fd8711ed9ac4..8f805b9f1c25 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -43,6 +43,12 @@ extern struct module __this_module;
 
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
 #include <linux/compiler.h>
+#ifdef CONFIG_64BIT
+#define __KSYMTAB_REL	".quad "
+#else
+#define __KSYMTAB_REL	".long "
+#endif
+
 /*
  * Emit the ksymtab entry as a pair of relative references: this reduces
  * the size by half on 64-bit architectures, and eliminates the need for
@@ -52,16 +58,16 @@ extern struct module __this_module;
 #define __KSYMTAB_ENTRY(sym, sec)					\
 	__ADDRESSABLE(sym)						\
 	asm("	.section \"___ksymtab" sec "+" #sym "\", \"a\"	\n"	\
-	    "	.balign	8					\n"	\
+	    "	.balign	4					\n"	\
 	    "__ksymtab_" #sym ":				\n"	\
-	    "	.long	" #sym "- .				\n"	\
+	    __KSYMTAB_REL #sym "- .				\n"	\
 	    "	.long	__kstrtab_" #sym "- .			\n"	\
 	    "	.previous					\n")
 
 struct kernel_symbol {
-	int value_offset;
+	long value_offset;
 	int name_offset;
-};
+} __packed;
 #else
 #define __KSYMTAB_ENTRY(sym, sec)					\
 	static const struct kernel_symbol __ksymtab_##sym		\
diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b3aaf5..43efd46feeee 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -541,7 +541,7 @@ static bool check_exported_symbol(const struct symsearch *syms,
 static unsigned long kernel_symbol_value(const struct kernel_symbol *sym)
 {
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-	return (unsigned long)offset_to_ptr(&sym->value_offset);
+	return (unsigned long)loffset_to_ptr(&sym->value_offset);
 #else
 	return sym->value;
 #endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread