From: Masahiro Yamada <masahiroy@kernel.org>
To: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
Nathan Chancellor <nathan@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Nicolas Schier <nicolas@fjasle.eu>,
linux-um@lists.infradead.org,
Masahiro Yamada <masahiroy@kernel.org>
Subject: [PATCH v8 01/11] Revert "[PATCH] uml: export symbols added by GCC hardened"
Date: Sat, 10 Jun 2023 18:13:10 +0900 [thread overview]
Message-ID: <20230610091320.1054554-2-masahiroy@kernel.org> (raw)
In-Reply-To: <20230610091320.1054554-1-masahiroy@kernel.org>
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. If no external library is linked, 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 and dangerous.
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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
(no changes since v7)
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
WARNING: multiple messages have this Message-ID (diff)
From: Masahiro Yamada <masahiroy@kernel.org>
To: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
Nathan Chancellor <nathan@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Nicolas Schier <nicolas@fjasle.eu>,
linux-um@lists.infradead.org,
Masahiro Yamada <masahiroy@kernel.org>
Subject: [PATCH v8 01/11] Revert "[PATCH] uml: export symbols added by GCC hardened"
Date: Sat, 10 Jun 2023 18:13:10 +0900 [thread overview]
Message-ID: <20230610091320.1054554-2-masahiroy@kernel.org> (raw)
In-Reply-To: <20230610091320.1054554-1-masahiroy@kernel.org>
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. If no external library is linked, 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 and dangerous.
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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
(no changes since v7)
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
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
next prev parent reply other threads:[~2023-06-10 9:13 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-10 9:13 [PATCH v8 00/11] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada [this message]
2023-06-10 9:13 ` [PATCH v8 01/11] Revert "[PATCH] uml: export symbols added by GCC hardened" Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 02/11] modpost: pass struct module pointer to check_section_mismatch() Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 03/11] kbuild: generate KSYMTAB entries by modpost Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 04/11] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL* Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 05/11] modpost: check static EXPORT_SYMBOL* by modpost again Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 06/11] modpost: squash sym_update_namespace() into sym_add_exported() Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 07/11] modpost: use null string instead of NULL pointer for default namespace Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 08/11] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 16:29 ` kernel test robot
2023-06-11 15:48 ` Masahiro Yamada
2023-06-10 16:40 ` kernel test robot
2023-06-10 18:24 ` kernel test robot
2023-06-11 15:50 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 09/11] modpost: merge two similar section mismatch warnings Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 10/11] modpost: show offset from symbol for " Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-10 9:13 ` [PATCH v8 11/11] linux/export.h: rename 'sec' argument to 'license' Masahiro Yamada
2023-06-10 9:13 ` Masahiro Yamada
2023-06-11 15:33 ` [PATCH v8 00/11] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
2023-06-11 15:33 ` Masahiro Yamada
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230610091320.1054554-2-masahiroy@kernel.org \
--to=masahiroy@kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-um@lists.infradead.org \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=nicolas@fjasle.eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.