Patch ubsan - Fix undefined references to ubsan functions

[Abhishek-612]

Resolves: Issue #6463

Added UBSan handlers for __ubsan_handle_type_mismatch_v1 and __ubsan_handle_pointer_overflow to remove the undefined references error.

Files affected:
core/kernel/ubsan.c

Additional Fixes:
Issue: UBSan causes a kernel panic with CFG_CORE_ASLR=y with a 'type_mismatch_v1' in boot.c -> get_aslr_seed() Cause: seed variable in get_asrl_seed() is not 8-byte aligned.
Fix: Before passing the seed to fdt64_to_cpu(), check if it is 8-byte aligned, and if not, make it so.
Affected file: core/arch/arm/kernel/boot.c

Signed-off-by: Abhishek Revadekar

[jenswi-linaro]

Please fix the checkpatch issues.

[jforissier]

Hi @Abhishek-brcm, thanks for the patch!

Please change the subject to "core: fix undefined references to ubsan functions".
We don't need the "Resolves:" nor the "Files affected:" lines.
Better write "Add UBSan handlers..." than "Added UBSan handlers" (imperative mood).
Your Signed-off-by: is missing an email address.
See my other comment below.

[jforissier]

You can't change the address of the seed. The correct fix is:

Suggested change

	/*
	Checks if seed is 8-byte aligned, and performs alignment if needed.
	If removed while CFG_CORE_SANITIZE_UNDEFINED is set, it may cause a ubsan panic
	*/
	if (((uint64_t)seed & 7) != 0) {
	seed = (uint64_t *)((unsigned long)seed + (8 - ((unsigned long)seed & 7)));
	}
	return fdt64_to_cpu(*seed);
	return fdt64_to_cpu(fdt64_ld(seed));

Please make this a separate fix ("core: arm: use fdt64_ld() to read possibly unaligned kaslr-seed").

[jforissier]

Once this is merged I'd like we have a QEMUv8 CI job with CFG_CORE_SANITIZE_UNDEFINED=y. There are at least two panics with UBSan enabled and maybe more. I am pasting the call stacks already in case someone would want to take a look.

$ xtest 4007_ed25519

E/TC:? 0 Undefined behavior shift_out_of_bounds at core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:55 col 12
E/TC:0 0 Panic at core/kernel/ubsan.c:193 <__ubsan_handle_shift_out_of_bounds>
E/TC:0 0 TEE load address @ 0x8e100000
E/TC:0 0 Call stack:
E/TC:0 0  0x8e10d6c0 print_kernel_stack at optee_os/core/arch/arm/kernel/unwind_arm64.c:89
E/TC:0 0  0x8e130c7c __do_panic at optee_os/core/kernel/panic.c:73
E/TC:0 0  0x8e134c9c __ubsan_handle_shift_out_of_bounds at optee_os/core/kernel/ubsan.c:193
E/TC:0 0  0x8e1cc740 car25519 at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:55 (discriminator 1)
E/TC:0 0  0x8e1cccbc M at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:135
E/TC:0 0  0x8e1cced8 add at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:261
E/TC:0 0  0x8e1ce594 scalarmult at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:294
E/TC:0 0  0x8e1ce6ac scalarbase at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:307
E/TC:0 0  0x8e1cf184 tweetnacl_crypto_sk_to_pk at optee_os/core/lib/libtomcrypt/src/pk/ec25519/tweetnacl.c:317
E/TC:0 0  0x8e1c063c ed25519_make_key at optee_os/core/lib/libtomcrypt/src/pk/ed25519/ed25519_make_key.c:25 (discriminator 2)
E/TC:0 0  0x8e1bfef8 crypto_acipher_gen_ed25519_key at optee_os/core/lib/libtomcrypt/ed25519.c:64
E/TC:0 0  0x8e16b228 tee_svc_obj_generate_key_ed25519 at optee_os/core/tee/tee_svc_cryp.c:2312
E/TC:0 0  0x8e108110 scall_do_call at optee_os/core/arch/arm/kernel/arch_scall_a64.S:140
E/TC:0 0  0x8e107544 thread_scall_handler at optee_os/core/arch/arm/kernel/thread.c:1138
E/TC:0 0  0x8e1048b4 el0_svc at optee_os/core/arch/arm/kernel/thread_a64.S:850

$ xtest 4013

E/TC:? 0 Undefined behavior nonnull_arg at core/kernel/user_access.c:56 col 3
E/TC:1 0 Panic at core/kernel/ubsan.c:241 <__ubsan_handle_nonnull_arg>
E/TC:1 0 TEE load address @ 0x8e100000
E/TC:1 0 Call stack:
E/TC:1 0  0x8e10d6c0 print_kernel_stack at optee_os/core/arch/arm/kernel/unwind_arm64.c:89
E/TC:1 0  0x8e130c7c __do_panic at optee_os/core/kernel/panic.c:73
E/TC:1 0  0x8e134eac __ubsan_handle_nonnull_arg at optee_os/core/kernel/ubsan.c:241
E/TC:1 0  0x8e135a48 copy_from_user at optee_os/core/kernel/user_access.c:56
E/TC:1 0  0x8e157198 system_derive_ta_unique_key at optee_os/core/pta/system.c:111
E/TC:1 0  0x8e141258 pseudo_ta_enter_invoke_cmd at optee_os/core/kernel/pseudo_ta.c:209
E/TC:1 0  0x8e13401c tee_ta_invoke_command at optee_os/core/kernel/tee_ta_manager.c:765
E/TC:1 0  0x8e162dac syscall_invoke_ta_command at optee_os/core/tee/tee_svc.c:871
E/TC:1 0  0x8e108110 scall_do_call at optee_os/core/arch/arm/kernel/arch_scall_a64.S:140
E/TC:1 0  0x8e107544 thread_scall_handler at optee_os/core/arch/arm/kernel/thread.c:1138
E/TC:1 0  0x8e1048b4 el0_svc at optee_os/core/arch/arm/kernel/thread_a64.S:850

[Abhishek-612]

Thanks for you feedback!
I have updated the commit message, and also removed the ASLR patch.

Should I create a separate PR for the latter?

[jforissier]

Thanks. No need for a separate PR, please make this one two patches, one to fix the ASLR seed and the other one to fix UBSan.

[Abhishek-612]

All fixes have been pushed to the branch.
Please let me know if there is any other feedback.

Thanks

[jforissier]

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Please squash the fixup patches. Thanks!

[jforissier]

Please address the checkpatch issues. Then for both commits:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

[etienne-lms]

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

[jenswi-linaro]

With the checkpatch issues fixed please apply:
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

[Abhishek-612]

checkpatch cannot find the Signed-off-by tag, but it is already present in both commits. Am I missing some formatting constraints?

[Abhishek-612]

There are at least two panics with UBSan enabled and maybe more. I am pasting the call stacks already in case someone would want to take a look.

Update on the xtest results:
I ran the full xtest suite. The two panics mentioned above (shift_out_of_bounds and nonnull_arg) are the only ones that were triggered throughout the tests.

I have prepared fixes for both issues. However, shift_out_of_bounds is triggered in libtomcrypt/src/pk/ec25519/tweetnacl.c. The comment description in the file says : " automatically generated file, do not edit ".

The panic can be avoided if we add a high bit mask during left shift operations (example below):

#define HIGH_16_MASK 0xFFFF000000000000

o[i]-=((HIGH_16_MASK ^ c) << 16);
// o[i]-=(c << 16);

Is it okay to patch this file?

[jforissier]

checkpatch cannot find the Signed-off-by tag, but it is already present in both commits. Am I missing some formatting constraints?

The commit 'Author' needs to match the first 'Signed-off-by'. To address this issue, you should first fix your Git configuration:

git config --global user.name "Abhishek Revadekar"
git config --global user.email "<abhishek.rvdkr@yahoo.com>"

Then amend the two commits:

git rebase -i HEAD^^
# replace "pick" with "e", save the rebase script, exit the editor
git commit --amend --reset-author
# While you are here, add the Reviewed-by/Acked-by given above
git rebase --continue
git commit --amend --reset-author
# Same here, add the R-b/A-b
git rebase --continue

Thanks!

[jforissier]

There are at least two panics with UBSan enabled and maybe more. I am pasting the call stacks already in case someone would want to take a look.

Update on the xtest results: I ran the full xtest suite. The two panics mentioned above (shift_out_of_bounds and nonnull_arg) are the only ones that were triggered throughout the tests.

I have prepared fixes for both issues.

Nice.

However, shift_out_of_bounds is triggered in libtomcrypt/src/pk/ec25519/tweetnacl.c. The comment description in the file says : " automatically generated file, do not edit ".

That file is imported from LibTomcrypt (https://github.com/libtom/libtomcrypt) and from what I can tell by looking at the file history in LibTomcrypt:

1. It is NOT generated but imported from https://tweetnacl.cr.yp.to/
2. Minor changes have been made to the file already (such as renaming symbols having a leading underscore).

Therefore I think it is safe to further edit the file.

The panic can be avoided if we add a high bit mask during left shift operations (example below):

#define HIGH_16_MASK 0xFFFF000000000000

o[i]-=((HIGH_16_MASK ^ c) << 16);
// o[i]-=(c << 16);

A better fix would be to make c a u64 rather than a i64, I think.

Is it okay to patch this file?

As I said above, yes I think so. My suggestion is to first fix the issue upstream first in the LibTomcrypt project by creating a pull request there and get feedback from the maintainers. In parallel also create a pull request here so that we can get the fix without having to wait for a full update of the library.
In the upstream pull request you may want to add a patch to remove the "automatically generated" line since it's confusing.

[Abhishek-612]

Thanks for the help!

Updated the commit messages.

[jforissier]

Please also fix the code style issues reported by checkpatch (alignment).

[Abhishek-612]

Can we ignore the WARNING: externs should be avoided in .c files?
There aren't any extern declaration for ubsan handlers in this repo.

[jforissier]

Can we ignore the WARNING: externs should be avoided in .c files? There aren't any extern declaration for ubsan handlers in this repo.

Yes, checkpatch.pl is confused, it is clearly a false positive.

[jforissier]

The alignment is still not good :/

[Abhishek-612]

Hopefully, the current version works.
Could you please confirm if these warnings are acceptable?

$ ./scripts/checkpatch.sh HEAD~1
Checking commit(s):
dba6aa476 core: fix undefined references to ubsan functions
No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory
WARNING: line length of 89 exceeds 80 columns
#24: FILE: core/kernel/ubsan.c:73:
+void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data, unsigned long ptr);

WARNING: externs should be avoided in .c files
#24: FILE: core/kernel/ubsan.c:73:
+void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data, unsigned long ptr);

WARNING: line length of 103 exceeds 80 columns
#32: FILE: core/kernel/ubsan.c:84:
+void __ubsan_handle_pointer_overflow(struct overflow_data *data, unsigned long lhs, unsigned long rhs);

WARNING: externs should be avoided in .c files
#32: FILE: core/kernel/ubsan.c:84:
+void __ubsan_handle_pointer_overflow(struct overflow_data *data, unsigned long lhs, unsigned long rhs);

WARNING: line length of 97 exceeds 80 columns
#40: FILE: core/kernel/ubsan.c:124:
+void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data, unsigned long ptr __unused)

WARNING: line length of 120 exceeds 80 columns
#54: FILE: core/kernel/ubsan.c:175:
+void __ubsan_handle_pointer_overflow(struct overflow_data *data, unsigned long lhs __unused, unsigned long rhs __unused)

total: 0 errors, 6 warnings, 0 checks, 40 lines checked

[jforissier]

No. Please wrap the lines correctly and with the proper alignment (tabs then spaces to align until after the opening parenthesis).
See attached patch.
0001-core-fix-undefined-references-to-ubsan-functions.patch.txt

[jforissier]

If you can't do it I can fix the patch manually when merging. My editor knows how to align code properly ;)

[Abhishek-612]

I think I got it this time!

1061884b4 core: fix undefined references to ubsan functions
No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory
WARNING: externs should be avoided in .c files
#24: FILE: core/kernel/ubsan.c:73:
+void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data,

WARNING: externs should be avoided in .c files
#33: FILE: core/kernel/ubsan.c:85:
+void __ubsan_handle_pointer_overflow(struct overflow_data *data,

total: 0 errors, 2 warnings, 0 checks, 45 lines checked

Should've used vi for the changes!