On Thu, Jun 8, 2023 at 7:24 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> 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@xxxxxxxxxx> Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> > --- > > 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 > -- Thanks, ~Nick Desaulniers
