Fix linking problems/warnings in ubsan #7198

alexhoppus · 2024-12-29T19:09:52Z

The following changes are done in ubsan subsystem:

__ubsan_handle_invalid_builtin callback was added. This ubsan handler
will be invoked when undefined behavior appears in __builtin_*
functions. That could happen if invalid arguments were provided.
E.g. passing 0 as the argument to __builtin_ctz_ or __builtin_clz_ invokes
undefined behavior and could be potentially caught by ubsan.
I was not able to build latest optee for vexpress-qemu_armv8a
(CFG_CORE_SANITIZE_UNDEFINED=y)
without this change, because this callback was generated in
libmbedtls code
It seems that signatures of ubsan functions are not compatible
with latest (or more or less fresh) gcc ubsan interfaces. That creates
warnings during compilation. These warnings were fixed.
It seems __ubsan_handle_unreachable should be __ubsan_handle_builtin_unreachable.
At least that corresponds to latest gcc and to what was there since version 4.9.0
Small refactoring to reduce code size

jenswi-linaro · 2025-01-02T13:38:18Z

core/kernel/ubsan.c

-					unsigned long lhs, unsigned long rhs);
-void __ubsan_handle_out_of_bounds(struct out_of_bounds_data *data,
-				  unsigned long idx);
+void __ubsan_handle_type_mismatch_v1(void *data_, void *ptr);


How did you find the proper declaration of this and the other __ubsan_handle_ functions?
Do you have a link?

Hi,

There are at least 3 ways

gcc gives warning during compilation and shows correct prototype at the same time. So one can rely on this gcc warning messages to figure out correct function signature. The initial idea behind this was to fix warnings.
For example:

core/kernel/ubsan.c:78:6: warning: conflicting types for built-in function ‘__ubsan_handle_type_mismatch_v1’; expected ‘void(void *, void *)’ [-Wbuiltin-declaration-mismatch] 78 | void __ubsan_handle_type_mismatch_v1(void *data_, unsigned long ptr);

one can refer to linux kernel implementation

look into gcc . I hope I gave a link to correct place

jenswi-linaro · 2025-01-02T13:41:30Z

core/kernel/ubsan.c

 	print_loc(__func__, &data->loc);
-	if (ubsan_panic)
-		panic();
+	panic();


Doesn't this result in a warning?

Hi,

I think the opposite results in a warning, since this function marked with attribute noreturn. So if it doesn't call explicitly any other noreturn function or doesn't end with infinite loop I get the warning.

core/kernel/ubsan.c: In function ‘__ubsan_handle_builtin_unreachable’: core/kernel/ubsan.c:188:1: warning: ‘noreturn’ function does return 188 | }

That is the reason why I treated noreturn functions in a little bit special way here.

That means that this function has an implicit noreturn attribute. How about adding it explicitly to be clear?

That means that this function has an implicit noreturn attribute. How about adding it explicitly to be clear?

That was exactly my thought and I tried to do this. But for some weird reason I get the warning message in this case

core/kernel/ubsan.c:182:1: warning: ignoring attribute ‘noreturn’ in declaration of a built-in function ‘__ubsan_handle_builtin_unreachable’ because it conflicts with attribute ‘const’ [-Wattributes] 182 | { | ^

After some research I found this. So I can mark it as noreturn, but we will see this warning. If you are okay, then I will do that.

Fair enough, it seems like a forever bug in GCC, let's keep it as it is.

jenswi-linaro · 2025-01-03T07:46:27Z

core/kernel/ubsan.c

+static volatile bool ubsan_panic = true;
+
+static void ubsan_handle_error(const char *func, struct source_location *loc,
+		bool should_panic)


Please align with the opening ( on the line above.

jenswi-linaro

Reviewed-by: Jens Wiklander <[email protected]>

alexhoppus · 2025-01-07T15:35:12Z

@jenswi-linaro hi, can you guide me please, what else should be done here? Thanks.

jforissier · 2025-01-07T15:45:26Z

Hi @alexhoppus

@jenswi-linaro hi, can you guide me please, what else should be done here? Thanks.

Nothing more, just wait for CI to complete. If it passes I will merge this PR. Thanks!

jforissier · 2025-01-07T16:14:10Z

Regarding the CI Code style errors:

I believe the "'externs" warnings are false positives, checkpatch seems to be confused. Let's ignore them.
Please fix the "unwrapped commit description" warning: https://github.com/OP-TEE/optee_os/actions/runs/12632861093/job/35262346722?pr=7198#step:5:109
See my comment about the "volatile"

Add UBSan handler __ubsan_handle_invalid_builtin, which support __builtin* functions validation. In some cases when __builtin functions are used, undefined behaviour might be triggered by invalid arguments. E.g. passing 0 as the argument to __builtin_ctz or __builtin_clz invokes undefined behavior and is diagnosed by UBSan. Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

Latest gcc versions utilize void * as argument type for most of the ubsan related handlers prototypes. Reproduced with gcc 11.2 . Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

There is only __ubsan_handle_builtin_unreachable interface in gcc. It was there starting from version 4.9.0. Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

Move panic invocation to common function. That makes entire code a little bit more compact and removes duplications. Also remove volatile modifier from ubsan_panic variable to make checkpatch happy. Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

alexhoppus · 2025-01-07T17:39:07Z

Thanks for the reply.

Regarding the CI Code style errors:

I believe the "'externs" warnings are false positives, checkpatch seems to be confused. Let's ignore them.

Please fix the "unwrapped commit description" warning: https://github.com/OP-TEE/optee_os/actions/runs/12632861093/job/35262346722?pr=7198#step:5:109

Fixed in this place and all other places, should be ok now.

See my comment about the "volatile"

I'm not sure which comment do you mean. The volatile variable, checkpatch complained to, was not introduced by me, but anyway I removed volatile modifier to make checkpatch happy. Per my understanding removing this volatile modifier shouldn't affect anything. I squashed this change directly to last commit, hope that is ok. Thanks.

jforissier · 2025-01-07T18:08:39Z

Regarding the CI Code style errors:

I believe the "'externs" warnings are false positives, checkpatch seems to be confused. Let's ignore them.
Please fix the "unwrapped commit description" warning: https://github.com/OP-TEE/optee_os/actions/runs/12632861093/job/35262346722?pr=7198#step:5:109
See my comment about the "volatile"

Thanks for the reply.

Regarding the CI Code style errors:

I believe the "'externs" warnings are false positives, checkpatch seems to be confused. Let's ignore them.

Please fix the "unwrapped commit description" warning: https://github.com/OP-TEE/optee_os/actions/runs/12632861093/job/35262346722?pr=7198#step:5:109

Fixed in this place and all other places, should be ok now.

See my comment about the "volatile"

I'm not sure which comment do you mean. The volatile variable, checkpatch complained to, was not introduced by me, but anyway I removed volatile modifier to make checkpatch happy. Per my understanding removing this volatile modifier shouldn't affect anything. I squashed this change directly to last commit, hope that is ok. Thanks.

Sorry I forgot to submit the review comment... but that's what I meant. Thanks.

jenswi-linaro · 2025-01-08T09:08:48Z

ubsan_panic was volatile to prevent the compiler from warning that a function didn't return. It looks like the extra indirection with a parameter to ubsan_handle_error() also does the trick.

jenswi-linaro reviewed Jan 2, 2025

View reviewed changes

jenswi-linaro reviewed Jan 3, 2025

View reviewed changes

alexhoppus force-pushed the ubsan-1 branch from 55f8496 to 2408521 Compare January 3, 2025 16:44

jenswi-linaro reviewed Jan 6, 2025

View reviewed changes

alexhoppus force-pushed the ubsan-1 branch from 2408521 to 349d7aa Compare January 6, 2025 12:53

alexhoppus added 4 commits January 7, 2025 18:08

core: fix gcc warnings in ubsan

c1846b0

Latest gcc versions utilize void * as argument type for most of the ubsan related handlers prototypes. Reproduced with gcc 11.2 . Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

core: fix error in ubsan_builtin_unreachable naming

64b8bf7

There is only __ubsan_handle_builtin_unreachable interface in gcc. It was there starting from version 4.9.0. Signed-off-by: Aleksandr Iashchenko <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

alexhoppus force-pushed the ubsan-1 branch from 349d7aa to 035a02c Compare January 7, 2025 17:35

jforissier merged commit 9f9c846 into OP-TEE:master Jan 8, 2025
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix linking problems/warnings in ubsan #7198

Fix linking problems/warnings in ubsan #7198

alexhoppus commented Dec 29, 2024

jenswi-linaro Jan 2, 2025

alexhoppus Jan 2, 2025

jenswi-linaro Jan 2, 2025

alexhoppus Jan 2, 2025

jenswi-linaro Jan 2, 2025

alexhoppus Jan 2, 2025 •

edited

Loading

jenswi-linaro Jan 2, 2025

jenswi-linaro Jan 3, 2025

alexhoppus Jan 3, 2025

jenswi-linaro left a comment

alexhoppus commented Jan 7, 2025

jforissier commented Jan 7, 2025

jforissier commented Jan 7, 2025

alexhoppus commented Jan 7, 2025 •

edited

Loading

jforissier commented Jan 7, 2025

jenswi-linaro commented Jan 8, 2025

Fix linking problems/warnings in ubsan #7198

Fix linking problems/warnings in ubsan #7198

Conversation

alexhoppus commented Dec 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexhoppus Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

alexhoppus commented Jan 7, 2025

jforissier commented Jan 7, 2025

jforissier commented Jan 7, 2025

alexhoppus commented Jan 7, 2025 • edited Loading

jforissier commented Jan 7, 2025

jenswi-linaro commented Jan 8, 2025

alexhoppus Jan 2, 2025 •

edited

Loading

alexhoppus commented Jan 7, 2025 •

edited

Loading