| Message ID | 20230608142428.256985-2-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS | expand |
On Thu, Jun 8, 2023 at 7:24 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > This reverts commit cead61a6717a9873426b08d73a34a325e3546f5d. > > It exported __stack_smash_handler and __guard, while they may not be > defined by anyone. > > The code *declares* __stack_smash_handler and __guard. It does not > create weak symbols. When the stack-protector is disabled, they are > left undefined, but yet exported. > > If a loadable module tries to access non-existing symbols, bad things > (a page fault, NULL pointer dereference, etc.) will happen. So, the > current code is wrong. > > If the code were written as follows, it would *define* them as weak > symbols so modules would be able to get access to them. > > void (*__stack_smash_handler)(void *) __attribute__((weak)); > EXPORT_SYMBOL(__stack_smash_handler); > > long __guard __attribute__((weak)); > EXPORT_SYMBOL(__guard); > > In fact, modpost forbids exporting undefined symbols. It shows an error > message if it detects such a mistake. > > ERROR: modpost: "..." [...] was exported without definition > > Unfortunately, it is checked only when the code is built as modular. > The problem described above has been unnoticed for a long time because > arch/um/os-Linux/user_syms.c is always built-in. > > With a planned change in Kbuild, exporting undefined symbols will always > result in a build error instead of a run-time error. It is a good thing, > but we need to fix the breakage in advance. > > One fix is to *define* weak symbols as shown above. An alternative is > to export them conditionally as follows: > > #ifdef CONFIG_STACKPROTECTOR > extern void __stack_smash_handler(void *); > EXPORT_SYMBOL(__stack_smash_handler); > > external long __guard; > EXPORT_SYMBOL(__guard); > #endif > > This is what other architectures do; EXPORT_SYMBOL(__stack_chk_guard) > is guarded by #ifdef CONFIG_STACKPROTECTOR. > > However, adding the #ifdef guard is not sensible because UML cannot > enable the stack-protector in the first place! (Please note UML does > not select HAVE_STACKPROTECTOR in Kconfig.) > > So, the code is already broken (and unused) in multiple ways. > > Just remove. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > > Changes in v7: > - New patch > > arch/um/os-Linux/user_syms.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c > index 9b62a9d352b3..a310ae27b479 100644 > --- a/arch/um/os-Linux/user_syms.c > +++ b/arch/um/os-Linux/user_syms.c > @@ -37,13 +37,6 @@ EXPORT_SYMBOL(vsyscall_ehdr); > EXPORT_SYMBOL(vsyscall_end); > #endif > > -/* Export symbols used by GCC for the stack protector. */ > -extern void __stack_smash_handler(void *) __attribute__((weak)); > -EXPORT_SYMBOL(__stack_smash_handler); > - > -extern long __guard __attribute__((weak)); > -EXPORT_SYMBOL(__guard); > - > #ifdef _FORTIFY_SOURCE > extern int __sprintf_chk(char *str, int flag, size_t len, const char *format); > EXPORT_SYMBOL(__sprintf_chk); > -- > 2.39.2 >
diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c index 9b62a9d352b3..a310ae27b479 100644 --- a/arch/um/os-Linux/user_syms.c +++ b/arch/um/os-Linux/user_syms.c @@ -37,13 +37,6 @@ EXPORT_SYMBOL(vsyscall_ehdr); EXPORT_SYMBOL(vsyscall_end); #endif -/* Export symbols used by GCC for the stack protector. */ -extern void __stack_smash_handler(void *) __attribute__((weak)); -EXPORT_SYMBOL(__stack_smash_handler); - -extern long __guard __attribute__((weak)); -EXPORT_SYMBOL(__guard); - #ifdef _FORTIFY_SOURCE extern int __sprintf_chk(char *str, int flag, size_t len, const char *format); EXPORT_SYMBOL(__sprintf_chk);
This reverts commit cead61a6717a9873426b08d73a34a325e3546f5d. It exported __stack_smash_handler and __guard, while they may not be defined by anyone. The code *declares* __stack_smash_handler and __guard. It does not create weak symbols. When the stack-protector is disabled, they are left undefined, but yet exported. If a loadable module tries to access non-existing symbols, bad things (a page fault, NULL pointer dereference, etc.) will happen. So, the current code is wrong. If the code were written as follows, it would *define* them as weak symbols so modules would be able to get access to them. void (*__stack_smash_handler)(void *) __attribute__((weak)); EXPORT_SYMBOL(__stack_smash_handler); long __guard __attribute__((weak)); EXPORT_SYMBOL(__guard); In fact, modpost forbids exporting undefined symbols. It shows an error message if it detects such a mistake. ERROR: modpost: "..." [...] was exported without definition Unfortunately, it is checked only when the code is built as modular. The problem described above has been unnoticed for a long time because arch/um/os-Linux/user_syms.c is always built-in. With a planned change in Kbuild, exporting undefined symbols will always result in a build error instead of a run-time error. It is a good thing, but we need to fix the breakage in advance. One fix is to *define* weak symbols as shown above. An alternative is to export them conditionally as follows: #ifdef CONFIG_STACKPROTECTOR extern void __stack_smash_handler(void *); EXPORT_SYMBOL(__stack_smash_handler); external long __guard; EXPORT_SYMBOL(__guard); #endif This is what other architectures do; EXPORT_SYMBOL(__stack_chk_guard) is guarded by #ifdef CONFIG_STACKPROTECTOR. However, adding the #ifdef guard is not sensible because UML cannot enable the stack-protector in the first place! (Please note UML does not select HAVE_STACKPROTECTOR in Kconfig.) So, the code is already broken (and unused) in multiple ways. Just remove. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- Changes in v7: - New patch arch/um/os-Linux/user_syms.c | 7 ------- 1 file changed, 7 deletions(-)