Provide a structure with the debug information to debug_halt() handler #470

romkuz01 · 2017-07-31T12:55:54Z

Ticket: https://jira.arm.com/browse/IOTSEC-369

I added a structure with the basic info that's supposed to aid debugging (some registers and exception stack frame when it's present).
A function debug_collect_halt_info() is used to fill this structure from within uVisor.
A new macro HALT_ERROR_EXTENDED() is used to issue halt with this debug information.

Out of the scope of this pull request: implementation of debug_halt() handler that accepts the debug information and prints it.
All the tests and examples that use debug box must be updated, this wasn't done yet.

niklarm · 2017-07-31T14:02:33Z

api/inc/halt_exports.h

+        uint32_t r0;
+        uint32_t r1;
+        uint32_t r2;
+        uint32_t r3;


(Check if for v8-M more registers are stacked and can be made available to the debug box by uVisor.)

On S to NS exception there may be more registers, but that doesn't seem to be our case.

niklarm · 2017-07-31T14:04:59Z

core/debug/src/debug.c

+
+    halt_info->valid_data = 0;
+    if (no_halt_info) {
+        return false;


Note to self: We don't want to print uVisor faults, since it could expose secrets. To debug uVisor faults users will have to use a uVisor debug build with semihosting.

Please extend the "For security reasons" comment with this information.

niklarm · 2017-07-31T14:06:11Z

api/inc/halt_exports.h

+        uint32_t lr;
+        uint32_t pc;
+        uint32_t xpsr;
+    } stack_frame;


Check if we haven't already defined a stack frame type somewhere in uVisor.

Changed a few files to use the same structure for the stack frame.

niklarm · 2017-07-31T14:18:57Z

core/debug/src/debug_box.c

+        if (halt_info) {
+            g_debug_interrupt_sp[g_debug_box.box_id] -= sizeof(THaltInfo);
+            info = (void *)g_debug_interrupt_sp[g_debug_box.box_id];
+            memcpy(info, halt_info, sizeof(THaltInfo));


This is okish. The alternative would be that the debug box reserves a THaltInfo in static memory and passes the pointer explicitly to uVisor either when registering the debug driver, or through the uVisor header input.
Note that memory consumption is the same, the additional memory on the stack has to be always considered.

Patater · 2017-07-31T17:18:58Z

api/inc/halt_exports.h

+        uint32_t xpsr;
+    } stack_frame;
+
+    /* A few registers that may be useful for debug. */


Try implementing the the BSOD printing code (debug_fault) in uVisor lib. Then, try calling the unprivileged BSOD printing code from the debug fault example (https://github.com/ARMmbed/mbed-os-example-uvisor-debug-fault). By attempting an implementation of the BSOD printing code, you should find out whether or not these registers (lr, control, ipsr) are useful for printing the debug BSOD.

Moving the printing code into a separate file (should be used by both core and lib) is relatively a big task. We agreed to file a separate ticket for it.

Patater · 2017-07-31T17:20:37Z

core/debug/src/debug.c

+
+    halt_info->valid_data = 0;
+    if (no_halt_info) {
+        return false;


Please extend the "For security reasons" comment with this information.

orenc17 · 2017-08-07T08:10:01Z

retest uvisor

romkuz01 · 2017-08-07T09:31:23Z

Oren hi, I forgot to mention one important thing - my commit breaks compilation on NUC42 due to flash image becoming to big. Alex is aware of that. Roman On 7 Aug 2017, at 11:10, Oren Cohen <notifications@github.com<mailto:notifications@github.com>> wrote: retest uvisor — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#470 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AWhOBZvdPQ1CvpgETY5GNczc9Qcisd5Zks5sVsZdgaJpZM4OoV2S>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

orenc17 · 2017-08-07T14:14:45Z

retest uvisor

orenc17 · 2017-08-07T14:59:58Z

retest uvisor

orenc17 · 2017-08-08T07:25:23Z

retest uvisor

Patater · 2017-08-10T08:21:36Z

Please follow our commit guidelines at https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/

Remove period from end of commit message titles
Start commit message titles with a capital letter
Describe the rationale for the change in the commit message description
Commits should be self-contained and cohesive
- Update uvisor-tests.txt pointer in the same commit that requires new tests
- Increase the maximum flash size before the commit that requires the increase (all commits should build on their own)
Commits should not link to nor contain non-public information

Please also rebase the commits in this PR on the latest master.

Patater · 2017-08-14T14:11:48Z

I get this error when building.

./core/vmpu/src/mpu_armv8m/vmpu_armv8m.c:125:9: error: implicit declaration of function 'debug_collect_halt_info' [-Werror=implicit-function-declaration]
     if (debug_collect_halt_info(lr, sp, &info)) {
         ^~~~~~~~~~~~~~~~~~~~~~~```

alzix · 2017-08-14T15:48:09Z

retest uvisor

orenc17 · 2017-08-14T16:28:55Z

fixed that

Patater · 2017-08-14T16:34:37Z

core/debug/src/debug.c

+     * don't have a valid exception stack frame.
+     * Stacking error may happen in case of Bus Fault, MemManage Fault or
+     * their escalation to Hard Fault. */
+    uint32_t stack_error_msk = SCB_CFSR_MSTKERR_Msk | SCB_CFSR_STKERR_Msk;


Make stack_error_msk "static const"

@Patater, what is the reason for making stack_error_msk static?

Without "static const", the compiler is likely to allocate the variable on the stack and copy the value from flash. This isn't necessary with constants, which can be used directly from flash.

Enhance debug box functionality. debug_halt now recieves a struct with the stack frame for better debugging.

niklarm reviewed Jul 31, 2017

View reviewed changes

Patater suggested changes Jul 31, 2017

View reviewed changes

romkuz01 force-pushed the romkuz01_dev branch 3 times, most recently from a430def to 8494b89 Compare August 6, 2017 09:11

orenc17 force-pushed the romkuz01_dev branch from ee98268 to 0e0d964 Compare August 7, 2017 13:14

orenc17 force-pushed the romkuz01_dev branch 3 times, most recently from 4022218 to 9c3c001 Compare August 14, 2017 13:14

orenc17 force-pushed the romkuz01_dev branch from 9c3c001 to f24fc4e Compare August 14, 2017 14:54

Patater reviewed Aug 14, 2017

View reviewed changes

orenc17 force-pushed the romkuz01_dev branch from f24fc4e to 487861a Compare August 14, 2017 16:36

Patater changed the title ~~Provide a structure with the debug information to debug_halt() handler.~~ Provide a structure with the debug information to debug_halt() handler Aug 14, 2017

orenc17 force-pushed the romkuz01_dev branch from 487861a to 50a6687 Compare August 14, 2017 16:44

Patater approved these changes Aug 14, 2017

View reviewed changes

orenc17 force-pushed the romkuz01_dev branch 2 times, most recently from 6f07f92 to 453db49 Compare August 15, 2017 07:25

Provide a structure with the debug information to debug_halt() handler

5f876d5

Enhance debug box functionality. debug_halt now recieves a struct with the stack frame for better debugging.

orenc17 force-pushed the romkuz01_dev branch from b247c14 to 5f876d5 Compare August 15, 2017 08:26

Patater merged commit aea9e2c into ARMmbed:master Aug 15, 2017

Provide a structure with the debug information to debug_halt() handler #470

Provide a structure with the debug information to debug_halt() handler #470

Uh oh!

Conversation

romkuz01 commented Jul 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenc17 commented Aug 7, 2017

Uh oh!

romkuz01 commented Aug 7, 2017 via email

Uh oh!

orenc17 commented Aug 7, 2017

Uh oh!

orenc17 commented Aug 7, 2017

Uh oh!

orenc17 commented Aug 8, 2017

Uh oh!

Patater commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Aug 14, 2017

Uh oh!

alzix commented Aug 14, 2017

Uh oh!

orenc17 commented Aug 14, 2017

Uh oh!

Patater Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Patater Jul 31, 2017 •

edited

Loading

Patater commented Aug 10, 2017 •

edited

Loading

Patater Aug 14, 2017 •

edited

Loading