os/board/bk7239n: Add Assertion log after TFM fault#7300
Open
pcs1265 wants to merge 1 commit intoSamsung:masterfrom
Open
os/board/bk7239n: Add Assertion log after TFM fault#7300pcs1265 wants to merge 1 commit intoSamsung:masterfrom
pcs1265 wants to merge 1 commit intoSamsung:masterfrom
Conversation
The CONFIG_TFM_S_TO_NS_DUMP_ENABLE is control the whole logging function when TFM crashes. But It's not tended to.
So this commit fixes misimplemented CONFIG_TFM_S_TO_NS_DUMP_ENABLE configuration, and add assertion log after TFM fault log.
Chnages:
Remove conditional compilation guards for CONFIG_TFM_S_TO_NS_DUMP_ENABLE
and CONFIG_SECURITY_LEVEL from the TFM dump callback registration and
secure fault handlers to ensure they are always included in the build.
Retain the CONFIG_TFM_S_TO_NS_DUMP_ENABLE guard around the
rtos_dump_system call and CONFIG_SECURITY_LEVEL around the high
security level check.
Replace bk_reboot_reset_reason() with PANIC() in secure fault handlers
to generate a proper panic trace instead of a silent reboot.
The valid scenarios are as follows:
- When CONFIG_TFM_S_TO_NS_DUMP_ENABLE=y
- When SECURITY_LEVEL is HIGH,
- The board should not print any crash info and reboot.
- Just call PANIC with setting reboot reason.; PANIC will print log or not by security level
- When SECURITY_LEVEL is LOW
- The board should print ALL crash info and reboot.
- Print all reg/memory dump and call PANIC with setting reboot reason.; PANIC will print log or not by security level
- When CONFIG_TFM_S_TO_NS_DUMP_ENABLE is not set
- When SECURITY_LEVEL is HIGH,
- The board should not print any crash info and reboot.
- Just call PANIC with setting reboot reason.; PANIC will print log or not by security level
- When SECURITY_LEVEL is LOW
- The board should print crash info summary (reg dump only. no memory dump) and reboot.
- Print reg dump only and call PANIC with setting reboot reason.; PANIC will print log or not by security level
pcs1265
force-pushed
the
260417_bk_tfm_reboot
branch
from
d943c9b to
5c7abf8
Compare
Member
Author
|
pcs1265@92f25bb#commitcomment-183016127 |
Contributor
PR #7300 — os/board/bk7239n: Add Assertion log after TFM fault
Repository: Base → Head: HEAD Commit: Scope: Review of the bk7239n TFM secure-fault dump path after unconditional callback registration and the switch from direct reboot to 🔎 Review Summary
Final Verdict: ❗ Request Changes 🚨 Must-Fix Issues1. Dump-disabled builds still emit the secure stack memory dump
Problem Impact
Evidence
Required Fix
Change outline:
Example patch: diff --git a/os/board/bk7239n/src/middleware/arch/cm33/trap_base.c b/os/board/bk7239n/src/middleware/arch/cm33/trap_base.c
@@
- uint32_t buflen = (sizeof(ctx->SE_EXC_FRAME_BUF) >>2);
-
- BK_DUMP_OUT(">>>>SE stack mem dump begin, stack_top=%08x, stack end=%08x\r\n", ctx->SE_EXC_FRAME_BUF, &(ctx->SE_EXC_FRAME_BUF[FRAME_BUF_LEN -1]));
- exception_frame_printf((uint32_t*)(ctx->SE_EXC_FRAME_BUF), buflen);
- BK_DUMP_OUT("<<<<SE stack mem dump end. stack_top=%08x, stack end=%08x\r\n", ctx->SE_EXC_FRAME_BUF, &(ctx->SE_EXC_FRAME_BUF[FRAME_BUF_LEN -1]));
+#ifdef CONFIG_TFM_S_TO_NS_DUMP_ENABLE
+ uint32_t buflen = (sizeof(ctx->SE_EXC_FRAME_BUF) >> 2);
+ BK_DUMP_OUT(">>>>SE stack mem dump begin, stack_top=%08x, stack end=%08x\r\n", ctx->SE_EXC_FRAME_BUF, &(ctx->SE_EXC_FRAME_BUF[FRAME_BUF_LEN - 1]));
+ exception_frame_printf((uint32_t *)ctx->SE_EXC_FRAME_BUF, buflen);
+ BK_DUMP_OUT("<<<<SE stack mem dump end. stack_top=%08x, stack end=%08x\r\n", ctx->SE_EXC_FRAME_BUF, &(ctx->SE_EXC_FRAME_BUF[FRAME_BUF_LEN - 1]));
+#endifInference:
Validation Method
🟡 Nice-to-Have Improvements1. Add explicit validation coverage for the new `CONFIG_TFM_S_TO_NS_DUMP_ENABLE=n` behavior
Problem Impact
Recommended Action
Expected Output
✅ Notable Improvements✔ Callback registration now matches the intended bk7239n default configuration
✔ The reboot reason is preserved before escalating into the common panic path
🧾 Final AssessmentMust-Fix Summary
Nice-to-Have Summary
Residual Risk / Test Gap
📌 Final Verdict
The reboot-handling direction looks better, but the patch still leaves the secure stack memory dump active when |
Contributor
Hello @pcs1265 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
CONFIG_TFM_S_TO_NS_DUMP_ENABLEcontrols the whole logging function when TFM crashes.But It's not intended to.
So this commit fixes misimplemented
CONFIG_TFM_S_TO_NS_DUMP_ENABLEconfiguration,and add assertion log after TFM fault log.
Chnages:
Remove conditional compilation guards for
CONFIG_TFM_S_TO_NS_DUMP_ENABLEandCONFIG_SECURITY_LEVELfrom the TFM dump callback registration and secure fault handlers to ensure they are always included in the build.Retain the
CONFIG_TFM_S_TO_NS_DUMP_ENABLEguard around the rtos_dump_system call andCONFIG_SECURITY_LEVELaround the high security level check.Replace
bk_reboot_reset_reason()withPANIC()in secure fault handlers to generate a proper panic trace instead of a silent reboot.The valid scenarios are as follows:
When CONFIG_TFM_S_TO_NS_DUMP_ENABLE=y
When CONFIG_TFM_S_TO_NS_DUMP_ENABLE is not set