Skip to contentSkip to conversation

ubsan: Avoid i386 UBSAN handler crashes with Clang

PATCH Superseded Kees Cook 2 participants 3 messages Apr 24, 2024

Subsystems (2)
Library code Ubsan

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 ubsan: Avoid i386 UBSAN handler crashes with Clang

When generating Runtime Calls, Clang doesn't respect the -mregparm=3 option used on i386. Hopefully this will be fixed correctly in Clang 19:
https://github.com/llvm/llvm-project/pull/89707
but we need to fix this for earlier Clang versions today. Force the calling convention to use non-register arguments.

Reported-by ernsteiswuerfel
Closes https://github.com/KSPP/linux/issues/350
Signed-off-by Kees Cook <[email protected]>

---
Cc: Marco Elver <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Bill Wendling <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 lib/ubsan.h | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)
lib/ubsan.h +27 -14
index 50ef50811b7c..978828f6099d 100644
@@ -124,19 +124,32 @@ typedef s64 s_max;
 typedef u64 u_max;
 #endif
 
-void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_negate_overflow(void *_data, void *old_val);
-void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
-void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
-void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
-void __ubsan_handle_out_of_bounds(void *_data, void *index);
-void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
-void __ubsan_handle_builtin_unreachable(void *_data);
-void __ubsan_handle_load_invalid_value(void *_data, void *val);
-void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
-					 unsigned long align,
-					 unsigned long offset);
+/*
+ * When generating Runtime Calls, Clang doesn't respect the -mregparm=3
+ * option used on i386. Hopefully this will be fixed correctly in Clang 19:
+ * https://github.com/llvm/llvm-project/pull/89707
+ * but we need to fix this for earlier Clang versions today. Force the
+ * calling convention to use non-register arguments.
+ */
+#if defined(__clang__) && defined(CONFIG_X86_32)
+# define ubsan_linkage asmlinkage
+#else
+# define ubsan_linkage /**/
+#endif
+
+void ubsan_linkage __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_negate_overflow(void *_data, void *old_val);
+void ubsan_linkage __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
+void ubsan_linkage __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
+void ubsan_linkage __ubsan_handle_out_of_bounds(void *_data, void *index);
+void ubsan_linkage __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_builtin_unreachable(void *_data);
+void ubsan_linkage __ubsan_handle_load_invalid_value(void *_data, void *val);
+void ubsan_linkage __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
+						       unsigned long align,
+						       unsigned long offset);
 
 #endif
Signature · 1 line
2.34.1
#1 lore raw

Hi Kees,

Quoted 7 lines · Kees Cook

When generating Runtime Calls, Clang doesn't respect the -mregparm=3 option used on i386. Hopefully this will be fixed correctly in Clang 19:
https://github.com/llvm/llvm-project/pull/89707
but we need to fix this for earlier Clang versions today. Force the calling convention to use non-register arguments.

Reported-by ernsteiswuerfel

FWIW, I think this can be

Reported-by Erhard Furtner <[email protected]>

since it has been used in the kernel before, the reporter is well known :)

Quoted 34 lines
Closes https://github.com/KSPP/linux/issues/350
Signed-off-by Kees Cook <[email protected]>

---
Cc: Marco Elver <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Bill Wendling <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 lib/ubsan.h | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)
lib/ubsan.h +0 -9
index 50ef50811b7c..978828f6099d 100644
@@ -124,19 +124,32 @@ typedef s64 s_max;
 typedef u64 u_max;
 #endif
 
-void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_negate_overflow(void *_data, void *old_val);
-void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
-void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
-void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
-void __ubsan_handle_out_of_bounds(void *_data, void *index);
-void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);

lib/ubsan.h typedef s64 s_max; · line 131

Quoted 10 lines
lib/ubsan.h +5 -5
-void __ubsan_handle_builtin_unreachable(void *_data);
-void __ubsan_handle_load_invalid_value(void *_data, void *val);
-void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
-					 unsigned long align,
-					 unsigned long offset);
+/*
+ * When generating Runtime Calls, Clang doesn't respect the -mregparm=3
+ * option used on i386. Hopefully this will be fixed correctly in Clang 19:
+ * https://github.com/llvm/llvm-project/pull/89707
+ * but we need to fix this for earlier Clang versions today. Force the

It may be better to link to the tracking issue upstream instead of the pull request just in case someone comes up with an alternative fix (not that I think your change is wrong or anything but it seems like that happens every so often).

I also get leary of the version information in the comment, even though I don't doubt this will be fixed in clang 19.

lib/ubsan.h typedef s64 s_max; · line 134

Quoted 3 lines
lib/ubsan.h +3 -0
+ * calling convention to use non-register arguments.
+ */
+#if defined(__clang__) && defined(CONFIG_X86_32)

While __clang__ is what causes CONFIG_CC_IS_CLANG to get set and there is some existing use of it throughout the kernel, I think CONFIG_CC_IS_CLANG makes it easier to audit the workarounds that we have, plus this will be presumably covered to

CONFIG_CLANG_VERSION < 190000

when the fix actually lands. This file is not expected to be used outside of the kernel, right? That is the only thing I could think of where this distinction would actually matter.

lib/ubsan.h typedef s64 s_max; · line 135

Quoted 1 line
lib/ubsan.h +1 -0
+# define ubsan_linkage asmlinkage

Heh, clever...

lib/ubsan.h typedef s64 s_max; · line 137

Quoted 2 lines
lib/ubsan.h +2 -0
+#else
+# define ubsan_linkage /**/

Why is this defined as a comment rather than just nothing?

Quoted 22 lines
lib/ubsan.h +16 -0
+#endif
+
+void ubsan_linkage __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_negate_overflow(void *_data, void *old_val);
+void ubsan_linkage __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
+void ubsan_linkage __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
+void ubsan_linkage __ubsan_handle_out_of_bounds(void *_data, void *index);
+void ubsan_linkage __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_builtin_unreachable(void *_data);
+void ubsan_linkage __ubsan_handle_load_invalid_value(void *_data, void *val);
+void ubsan_linkage __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
+						       unsigned long align,
+						       unsigned long offset);
 
 #endif
Signature · 1 line
2.34.1
author
#2 lore raw
Quoted 9 lines · Nathan Chancellor

Hi Kees,

Quoted 6 lines · Kees Cook

When generating Runtime Calls, Clang doesn't respect the -mregparm=3 option used on i386. Hopefully this will be fixed correctly in Clang 19:
https://github.com/llvm/llvm-project/pull/89707
but we need to fix this for earlier Clang versions today. Force the calling convention to use non-register arguments.

Quoted 8 lines
Quoted 1 line
Reported-by ernsteiswuerfel

FWIW, I think this can be

Reported-by Erhard Furtner <[email protected]>

since it has been used in the kernel before, the reporter is well known :)

Ah! Okay, thanks. I wasn't able to find an associated email address. :)

Quoted 58 lines
Quoted 35 lines
Closes https://github.com/KSPP/linux/issues/350
Signed-off-by Kees Cook <[email protected]>

---
Cc: Marco Elver <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Bill Wendling <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
 lib/ubsan.h | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)
lib/ubsan.h +0 -9
index 50ef50811b7c..978828f6099d 100644
@@ -124,19 +124,32 @@ typedef s64 s_max;
 typedef u64 u_max;
 #endif
 
-void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
-void __ubsan_handle_negate_overflow(void *_data, void *old_val);
-void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
-void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
-void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
-void __ubsan_handle_out_of_bounds(void *_data, void *index);
-void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);

lib/ubsan.h typedef s64 s_max; · line 131

Quoted 10 lines
lib/ubsan.h +5 -5
-void __ubsan_handle_builtin_unreachable(void *_data);
-void __ubsan_handle_load_invalid_value(void *_data, void *val);
-void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
-					 unsigned long align,
-					 unsigned long offset);
+/*
+ * When generating Runtime Calls, Clang doesn't respect the -mregparm=3
+ * option used on i386. Hopefully this will be fixed correctly in Clang 19:
+ * https://github.com/llvm/llvm-project/pull/89707
+ * but we need to fix this for earlier Clang versions today. Force the

It may be better to link to the tracking issue upstream instead of the pull request just in case someone comes up with an alternative fix (not that I think your change is wrong or anything but it seems like that happens every so often).

I also get leary of the version information in the comment, even though I don't doubt this will be fixed in clang 19.

lib/ubsan.h typedef s64 s_max; · line 134

Quoted 3 lines
lib/ubsan.h +3 -0
+ * calling convention to use non-register arguments.
+ */
+#if defined(__clang__) && defined(CONFIG_X86_32)
Quoted 6 lines

While __clang__ is what causes CONFIG_CC_IS_CLANG to get set and there is some existing use of it throughout the kernel, I think CONFIG_CC_IS_CLANG makes it easier to audit the workarounds that we have, plus this will be presumably covered to

CONFIG_CLANG_VERSION < 190000

Yeah, that seems much cleaner. I will adjust it...

Quoted 5 lines

when the fix actually lands. This file is not expected to be used outside of the kernel, right? That is the only thing I could think of where this distinction would actually matter.

Quoted 8 lines

lib/ubsan.h typedef s64 s_max; · line 135

Quoted 1 line
lib/ubsan.h +1 -0
+# define ubsan_linkage asmlinkage

Heh, clever...

lib/ubsan.h typedef s64 s_max; · line 137

Quoted 2 lines
lib/ubsan.h +2 -0
+#else
+# define ubsan_linkage /**/

Why is this defined as a comment rather than just nothing?

I dunno; this is a coding style glitch of mine. :P I will drop it.

Thanks for the review!

-Kees

Quoted 23 lines
Quoted 23 lines
lib/ubsan.h +16 -0
+#endif
+
+void ubsan_linkage __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_negate_overflow(void *_data, void *old_val);
+void ubsan_linkage __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
+void ubsan_linkage __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
+void ubsan_linkage __ubsan_handle_out_of_bounds(void *_data, void *index);
+void ubsan_linkage __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
+void ubsan_linkage __ubsan_handle_builtin_unreachable(void *_data);
+void ubsan_linkage __ubsan_handle_load_invalid_value(void *_data, void *val);
+void ubsan_linkage __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
+						       unsigned long align,
+						       unsigned long offset);
 
 #endif
Signature · 1 line
2.34.1
Signature · 1 line
Kees Cook