LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
@ 2019-05-22 15:02 Ard Biesheuvel
  2019-05-22 16:28 ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ 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	[flat|nested] 9+ messages in thread

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-22 15:02 [PATCH] module/ksymtab: use 64-bit relative reference for target symbol Ard Biesheuvel
@ 2019-05-22 16:28 ` Ard Biesheuvel
  2019-05-23  8:41   ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2019-05-22 16:28 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



On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> 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.
> 

Unfortunately, this is not quite true. In addition to that patch, we 
need some changes to the x86 'relocs' tool so it can handle 64-bit 
relative references to per-CPU symbols, much like the support it has 
today for 32-bit relative references. I have coded it up, and will send 
it out as soon as I have confirmed that it works.


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

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-22 16:28 ` Ard Biesheuvel
@ 2019-05-23  8:41   ` Ard Biesheuvel
  2019-05-23  9:18     ` Will Deacon
  0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2019-05-23  8:41 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



On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> 
> 
> On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
>> 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.
>>

(Apologies for the 3-act monologue)

This turns out to be incorrect. The symbol address of per-CPU variables 
exported by modules is always in the vicinity of __per_cpu_start, and so 
it is simply a matter of making sure that the core kernel is in range 
for module ksymtab entries containing 32-bit relative references.

When running the arm64 with kaslr enabled, we currently randomize the 
module space based on the range of ADRP/ADD instruction pairs, which 
have a -/+ 4 GB range rather than the -/+ 2 GB range of 32-bit place 
relative data relocations. So we can fix this by simply reducing the 
randomization window to 2 GB.

So please disregard this patch (and the followup one against arch/x86/tools)

-- 
Ard.

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-23  8:41   ` Ard Biesheuvel
@ 2019-05-23  9:18     ` Will Deacon
  2019-05-23  9:29       ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2019-05-23  9:18 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-arm-kernel, marc.zyngier, james.morse, guillaume.gardet,
	mark.rutland, mingo, jeyu, linux-kernel, linux-arch, arnd, x86

On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
> 
> 
> On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> > 
> > 
> > On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> > > 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.
> > > 
> 
> (Apologies for the 3-act monologue)

Exposition, development and recapitulation ;)

> This turns out to be incorrect. The symbol address of per-CPU variables
> exported by modules is always in the vicinity of __per_cpu_start, and so it
> is simply a matter of making sure that the core kernel is in range for
> module ksymtab entries containing 32-bit relative references.
> 
> When running the arm64 with kaslr enabled, we currently randomize the module
> space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
> GB range rather than the -/+ 2 GB range of 32-bit place relative data
> relocations. So we can fix this by simply reducing the randomization window
> to 2 GB.

Makes sense. Do you see the need for an option to disable PREL relocs
altogether in case somebody wants the additional randomization range?

Will

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-23  9:18     ` Will Deacon
@ 2019-05-23  9:29       ` Ard Biesheuvel
  2019-05-24 15:20         ` Josh Poimboeuf
  0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2019-05-23  9:29 UTC (permalink / raw)
  To: Will Deacon
  Cc: linux-arm-kernel, marc.zyngier, james.morse, guillaume.gardet,
	mark.rutland, mingo, jeyu, linux-kernel, linux-arch, arnd, x86



On 5/23/19 10:18 AM, Will Deacon wrote:
> On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
>>
>>
>> On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
>>>
>>>
>>> On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
>>>> 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.
>>>>
>>
>> (Apologies for the 3-act monologue)
> 
> Exposition, development and recapitulation ;)
> 
>> This turns out to be incorrect. The symbol address of per-CPU variables
>> exported by modules is always in the vicinity of __per_cpu_start, and so it
>> is simply a matter of making sure that the core kernel is in range for
>> module ksymtab entries containing 32-bit relative references.
>>
>> When running the arm64 with kaslr enabled, we currently randomize the module
>> space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
>> GB range rather than the -/+ 2 GB range of 32-bit place relative data
>> relocations. So we can fix this by simply reducing the randomization window
>> to 2 GB.
> 
> Makes sense. Do you see the need for an option to disable PREL relocs
> altogether in case somebody wants the additional randomization range?
> 

No, not really. To be honest, I don't think 
CONFIG_RANDOMIZE_MODULE_REGION_FULL is that useful to begin with, and 
the only reason we enabled it by default at the time was to ensure that 
the PLT code got some coverage after we introduced it.



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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-23  9:29       ` Ard Biesheuvel
@ 2019-05-24 15:20         ` Josh Poimboeuf
  2019-05-24 15:55           ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Josh Poimboeuf @ 2019-05-24 15:20 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Will Deacon, linux-arm-kernel, marc.zyngier, james.morse,
	guillaume.gardet, mark.rutland, mingo, jeyu, linux-kernel,
	linux-arch, arnd, x86

On Thu, May 23, 2019 at 10:29:39AM +0100, Ard Biesheuvel wrote:
> 
> 
> On 5/23/19 10:18 AM, Will Deacon wrote:
> > On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
> > > 
> > > 
> > > On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> > > > 
> > > > 
> > > > On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> > > > > 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.
> > > > > 
> > > 
> > > (Apologies for the 3-act monologue)
> > 
> > Exposition, development and recapitulation ;)
> > 
> > > This turns out to be incorrect. The symbol address of per-CPU variables
> > > exported by modules is always in the vicinity of __per_cpu_start, and so it
> > > is simply a matter of making sure that the core kernel is in range for
> > > module ksymtab entries containing 32-bit relative references.
> > > 
> > > When running the arm64 with kaslr enabled, we currently randomize the module
> > > space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
> > > GB range rather than the -/+ 2 GB range of 32-bit place relative data
> > > relocations. So we can fix this by simply reducing the randomization window
> > > to 2 GB.
> > 
> > Makes sense. Do you see the need for an option to disable PREL relocs
> > altogether in case somebody wants the additional randomization range?
> > 
> 
> No, not really. To be honest, I don't think
> CONFIG_RANDOMIZE_MODULE_REGION_FULL is that useful to begin with, and the
> only reason we enabled it by default at the time was to ensure that the PLT
> code got some coverage after we introduced it.

In code, percpu variables are accessed with absolute relocations, right?
Before I read your 3rd act, I was wondering if it would make sense to do
the same with the ksymtab relocations.

Like if we somehow [ insert much hand waving ] ensured that everybody
uses EXPORT_PER_CPU_SYMBOL() for percpu symbols instead of just
EXPORT_SYMBOL() then we could use a different macro to create the
ksymtab relocations for percpu variables, such that they use absolute
relocations.

Just an idea.  Maybe the point is moot now.

-- 
Josh

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-24 15:20         ` Josh Poimboeuf
@ 2019-05-24 15:55           ` Ard Biesheuvel
  2019-05-24 16:31             ` Josh Poimboeuf
  0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2019-05-24 15:55 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Ard Biesheuvel, Mark Rutland, linux-arch, Arnd Bergmann,
	guillaume.gardet, Marc Zyngier, the arch/x86 maintainers,
	Will Deacon, Linux Kernel Mailing List, James Morse, Jessica Yu,
	Ingo Molnar, linux-arm-kernel

On Fri, 24 May 2019 at 17:21, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, May 23, 2019 at 10:29:39AM +0100, Ard Biesheuvel wrote:
> >
> >
> > On 5/23/19 10:18 AM, Will Deacon wrote:
> > > On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
> > > >
> > > >
> > > > On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> > > > >
> > > > >
> > > > > On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> > > > > > 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.
> > > > > >
> > > >
> > > > (Apologies for the 3-act monologue)
> > >
> > > Exposition, development and recapitulation ;)
> > >
> > > > This turns out to be incorrect. The symbol address of per-CPU variables
> > > > exported by modules is always in the vicinity of __per_cpu_start, and so it
> > > > is simply a matter of making sure that the core kernel is in range for
> > > > module ksymtab entries containing 32-bit relative references.
> > > >
> > > > When running the arm64 with kaslr enabled, we currently randomize the module
> > > > space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
> > > > GB range rather than the -/+ 2 GB range of 32-bit place relative data
> > > > relocations. So we can fix this by simply reducing the randomization window
> > > > to 2 GB.
> > >
> > > Makes sense. Do you see the need for an option to disable PREL relocs
> > > altogether in case somebody wants the additional randomization range?
> > >
> >
> > No, not really. To be honest, I don't think
> > CONFIG_RANDOMIZE_MODULE_REGION_FULL is that useful to begin with, and the
> > only reason we enabled it by default at the time was to ensure that the PLT
> > code got some coverage after we introduced it.
>
> In code, percpu variables are accessed with absolute relocations, right?

No, they are accessed just like ordinary symbols, so PC32 references
on x86 or ADRP/ADD references on arm64 are both quite common.

> Before I read your 3rd act, I was wondering if it would make sense to do
> the same with the ksymtab relocations.
>
> Like if we somehow [ insert much hand waving ] ensured that everybody
> uses EXPORT_PER_CPU_SYMBOL() for percpu symbols instead of just
> EXPORT_SYMBOL() then we could use a different macro to create the
> ksymtab relocations for percpu variables, such that they use absolute
> relocations.
>
> Just an idea.  Maybe the point is moot now.
>

The problem is that we already have four different ksymtab sections:
normal, GPL, future GPL and unused, and adding the orthogonal per-CPU
property to that would double it to 8.

Since the purpose of the place relative ksymtabs applies to the core
kernel only, another thing I contemplated is using a different ksymtab
format between modules and the core kernel, but that is another can of
worms that I'd rather not open.

But it is indeed moot now ...

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-24 15:55           ` Ard Biesheuvel
@ 2019-05-24 16:31             ` Josh Poimboeuf
  2019-05-24 16:33               ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Josh Poimboeuf @ 2019-05-24 16:31 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Ard Biesheuvel, Mark Rutland, linux-arch, Arnd Bergmann,
	guillaume.gardet, Marc Zyngier, the arch/x86 maintainers,
	Will Deacon, Linux Kernel Mailing List, James Morse, Jessica Yu,
	Ingo Molnar, linux-arm-kernel

On Fri, May 24, 2019 at 05:55:37PM +0200, Ard Biesheuvel wrote:
> On Fri, 24 May 2019 at 17:21, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Thu, May 23, 2019 at 10:29:39AM +0100, Ard Biesheuvel wrote:
> > >
> > >
> > > On 5/23/19 10:18 AM, Will Deacon wrote:
> > > > On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
> > > > >
> > > > >
> > > > > On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> > > > > >
> > > > > >
> > > > > > On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> > > > > > > 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.
> > > > > > >
> > > > >
> > > > > (Apologies for the 3-act monologue)
> > > >
> > > > Exposition, development and recapitulation ;)
> > > >
> > > > > This turns out to be incorrect. The symbol address of per-CPU variables
> > > > > exported by modules is always in the vicinity of __per_cpu_start, and so it
> > > > > is simply a matter of making sure that the core kernel is in range for
> > > > > module ksymtab entries containing 32-bit relative references.
> > > > >
> > > > > When running the arm64 with kaslr enabled, we currently randomize the module
> > > > > space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
> > > > > GB range rather than the -/+ 2 GB range of 32-bit place relative data
> > > > > relocations. So we can fix this by simply reducing the randomization window
> > > > > to 2 GB.
> > > >
> > > > Makes sense. Do you see the need for an option to disable PREL relocs
> > > > altogether in case somebody wants the additional randomization range?
> > > >
> > >
> > > No, not really. To be honest, I don't think
> > > CONFIG_RANDOMIZE_MODULE_REGION_FULL is that useful to begin with, and the
> > > only reason we enabled it by default at the time was to ensure that the PLT
> > > code got some coverage after we introduced it.
> >
> > In code, percpu variables are accessed with absolute relocations, right?
> 
> No, they are accessed just like ordinary symbols, so PC32 references
> on x86 or ADRP/ADD references on arm64 are both quite common.

Ah, right, now I see some PC32 percpu references.

So if PC32 references are sufficient for code, why aren't they
sufficient for ksymtab entries?  Isn't the ksymtab data address closer
to the percpu data than the code?  Do you have an example of an out of
range ksymtab reference?

-- 
Josh

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

* Re: [PATCH] module/ksymtab: use 64-bit relative reference for target symbol
  2019-05-24 16:31             ` Josh Poimboeuf
@ 2019-05-24 16:33               ` Ard Biesheuvel
  0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2019-05-24 16:33 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Ard Biesheuvel, Mark Rutland, linux-arch, Arnd Bergmann,
	guillaume.gardet, Marc Zyngier, the arch/x86 maintainers,
	Will Deacon, Linux Kernel Mailing List, James Morse, Jessica Yu,
	Ingo Molnar, linux-arm-kernel

On Fri, 24 May 2019 at 18:31, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, May 24, 2019 at 05:55:37PM +0200, Ard Biesheuvel wrote:
> > On Fri, 24 May 2019 at 17:21, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Thu, May 23, 2019 at 10:29:39AM +0100, Ard Biesheuvel wrote:
> > > >
> > > >
> > > > On 5/23/19 10:18 AM, Will Deacon wrote:
> > > > > On Thu, May 23, 2019 at 09:41:40AM +0100, Ard Biesheuvel wrote:
> > > > > >
> > > > > >
> > > > > > On 5/22/19 5:28 PM, Ard Biesheuvel wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 5/22/19 4:02 PM, Ard Biesheuvel wrote:
> > > > > > > > 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.
> > > > > > > >
> > > > > >
> > > > > > (Apologies for the 3-act monologue)
> > > > >
> > > > > Exposition, development and recapitulation ;)
> > > > >
> > > > > > This turns out to be incorrect. The symbol address of per-CPU variables
> > > > > > exported by modules is always in the vicinity of __per_cpu_start, and so it
> > > > > > is simply a matter of making sure that the core kernel is in range for
> > > > > > module ksymtab entries containing 32-bit relative references.
> > > > > >
> > > > > > When running the arm64 with kaslr enabled, we currently randomize the module
> > > > > > space based on the range of ADRP/ADD instruction pairs, which have a -/+ 4
> > > > > > GB range rather than the -/+ 2 GB range of 32-bit place relative data
> > > > > > relocations. So we can fix this by simply reducing the randomization window
> > > > > > to 2 GB.
> > > > >
> > > > > Makes sense. Do you see the need for an option to disable PREL relocs
> > > > > altogether in case somebody wants the additional randomization range?
> > > > >
> > > >
> > > > No, not really. To be honest, I don't think
> > > > CONFIG_RANDOMIZE_MODULE_REGION_FULL is that useful to begin with, and the
> > > > only reason we enabled it by default at the time was to ensure that the PLT
> > > > code got some coverage after we introduced it.
> > >
> > > In code, percpu variables are accessed with absolute relocations, right?
> >
> > No, they are accessed just like ordinary symbols, so PC32 references
> > on x86 or ADRP/ADD references on arm64 are both quite common.
>
> Ah, right, now I see some PC32 percpu references.
>
> So if PC32 references are sufficient for code, why aren't they
> sufficient for ksymtab entries?  Isn't the ksymtab data address closer
> to the percpu data than the code?  Do you have an example of an out of
> range ksymtab reference?
>

Not on x86, only on arm64, which uses ADRP/ADD pairs with a -/+ 4 GB
range as opposed to the -/+ 2 GB range of PC32 and PREL32 references.
So when KASLR puts the modules far away from the kernel (but in range
for ADRP/ADD) they may be out of range for PREL32.

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

end of thread, other threads:[~2019-05-24 16:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-22 15:02 [PATCH] module/ksymtab: use 64-bit relative reference for target symbol Ard Biesheuvel
2019-05-22 16:28 ` Ard Biesheuvel
2019-05-23  8:41   ` Ard Biesheuvel
2019-05-23  9:18     ` Will Deacon
2019-05-23  9:29       ` Ard Biesheuvel
2019-05-24 15:20         ` Josh Poimboeuf
2019-05-24 15:55           ` Ard Biesheuvel
2019-05-24 16:31             ` Josh Poimboeuf
2019-05-24 16:33               ` Ard Biesheuvel

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 NNTP newsgroup(s).