Add deprivilege/print/reprivilege flow by romkuz01 · Pull Request #497 · ARMmbed/uvisor

romkuz01 · 2017-08-29T14:48:58Z

Major changes:

Rename old debug_deprivilege_and_return() to debug_deprivilege_and_die()
which better reflects its nature
Add a new debug_deprivilege_and_return() which indeed
performs deprivilege and return within the function itself
Add a new API function uvisor_debug_print_enable() which should
be used from the user's application to enable
deprivilege/print/reprivilege flow
Make SVC's priority higher than of the other exceptions with programmable priority
Normalize line endings used in printing in a few places to "\r\n" (still need to fix in some other places)

Note:
Ideally it would be better to break this commit into a few smaller ones

Usage example in the user's application:
...
static void halt_error(THaltError reason, const THaltInfo * halt_info)
{
...
}

static void debug_print(const char * message)
{
printf("%s", message);
}

UVISOR_PUBLIC_BOX_DEBUG_DRIVER(halt_error, debug_print);

#define BAD_BAD_ADDR (*((volatile unsigned long *) (0xFFFFFFFF)))

int main(void)
{
uvisor_debug_print_enable(true);

/* About to get a fault now: */
    BAD_BAD_ADDR = 13;

}

alzix

i have not yet had time to review the whole thing - these are my initial thoughts

alzix · 2017-08-29T17:41:51Z

-    DPRINTF("\n***********************************************************\n");\
-    DPRINTF("                    "x"\n");\
-    DPRINTF("***********************************************************\n\n");\
+    DPRINTF("\r\n***********************************************************\r\n");\


please remove all \r and configure your serial terminal to force CR on each LF - it is a terminal configuration and should not be treated in code.

alzix · 2017-08-29T17:47:13Z

    void (*spin_unlock)(UvisorSpinlock * spinlock);

    void (*debug_semihosting_enable)(void);
+    void (*debug_print_enable)(bool);


this function comes to solve initialization order problems when using debug driver.
I believe this logic should reside on application side. An alternative is to look for any falg/callback from mbed-os - for checking such a condition.

Application can define a global bool debug_print_enable = false; variable, and avoid printing if it is disabled.
as i see it:

bool debug_print_enable = false; void debug_print_driver(THaltError error_code, const THaltInfo *bsod) { if (debug_print_enable){ printf("oops\n"); } } int main(void) { debug_print_enable = true; ... }

alzix · 2017-08-29T17:51:11Z


 UVISOR_WEAK void default_putc(uint8_t data)
 {
-    if (DEBUG_SEMIHOSTING_MAGIC == g_semihosting_magic) {


i suggest not to remove semi-hosting print logic - we still need it for debugging stuff during early boot stages.
we can duplicate print to two sinks - that is what all decent loggers do :)

Not removed, divided into two separate checks.

This better reflects the nature of this function. The real debug_deprivilege_and_return() will be added later.

Since we're not switching back to the same point this copying makes no sense. Moreover, it can create problematic behavior if ICI/IT field is set to non-zero state on the target stack frame during the copying.

Different line endings were used throughout the project - \n, \r\n and \n\r. All of them were set to be \n. Also dprintf("\n\r") and dprintf("\n") were replaced by default_putc('\n'). The implications are that any COM port monitors must be configured to treat \n as \r\n to maintain correct printout layout.

Patater

I quite like calling SVC to enter the debug box. Excellent approach! Nice work.

I have a few questions about the implementation. Please find the comments inline with the source code.

Patater · 2017-08-31T12:59:53Z

+        "push {r4 - r11}\n"
+
+        /* Execute SVC that will perform the deprivileging. */
+    	"svc %[depriv]\n"


Do we ever call debug_deprivilege_and_return from while an SVC is active? I suspect not, but if we did, we'd have to clear SVC active bit before calling the SVC here, and restore it after return.

Right now we can't perform deprivileging from SVC handler since executing SVC in this case will result in Hard Fault.
We need either disable deprivileging from SVC like it's done for Hard Fault/NMI or manually play with active bit. I'll check this option.

I added active bit cleaning, so now deprivileging from SVC handler is possible.

Patater · 2017-08-31T13:03:25Z

+    /* Save the current context on the stack, deprivilege and restore the context upon return. */
+    asm volatile(
+        /* Save general purpose registers that won't be saved by the following SVC. */
+        "push {r4 - r11}\n"


Do we also need to include lr?

{r4-r11, lr}

No need since LR is automatically saved when executing SVC after this PUSH.
Also R0-R3, R12 and xPSR.

Patater · 2017-08-31T13:04:16Z


    /* De-privilege, call the debug box handler, re-privilege, call the return
     * handler. */
+    /* FIXME: the below way of deprivileging may be problematic when executed from an exception handler


Patater · 2017-08-31T13:04:35Z

    ((void (*)(void)) return_handler)();
 }
+
+/* FIXME: replace these stubs by the actual implementation. */


Is the plan to implement these in this PR or a later one?

Probably it's better to do it in a separate PR.
This one is already big enough.

Can we remove these empty stubs or does that break the build?

Breaks the build.

Patater · 2017-08-31T13:07:15Z

+{
+	/* Abort printing if debug box wasn't initialized yet. */
+	if (!g_debug_box.initialized) {
+		return;


How often do you run into this case in practice? I'm curious what prints we might be missing out on. With the statically initialized debug box, I'd hope we could get prints even very early on.

Currently the initialization header isn't printed out:

uVisor mode: 2
uvisor_ram : @0x1FFF0400 (7168 bytes) [config]
@0x1FFF0400 (7168 bytes) [linker]
bss_boxes : @0x1FFF2000 (128 bytes) [config]
Box 0 ACLs:

Peripheral: 0x40047000 - 0x40048064 (permissions: 0x0AB6)

Peripheral: 0x40065000 - 0x40065001 (permissions: 0x0AB6)

Peripheral: 0x40064000 - 0x4006400E (permissions: 0x0AB6)

Peripheral: 0x40049000 - 0x400490CC (permissions: 0x0AB6)

Peripheral: 0x4004A000 - 0x4004A0CC (permissions: 0x0AB6)

Peripheral: 0x4004B000 - 0x4004B0CC (permissions: 0x0AB6)

Peripheral: 0x4004C000 - 0x4004C0CC (permissions: 0x0AB6)

Peripheral: 0x4004D000 - 0x4004D0CC (permissions: 0x0AB6)

Peripheral: 0x4003D000 - 0x4003D808 (permissions: 0x0AB6)

Peripheral: 0x40040000 - 0x40040010 (permissions: 0x0AB6)

Peripheral: 0x40037000 - 0x40037140 (permissions: 0x0AB6)

Peripheral: 0x4007E000 - 0x4007E004 (permissions: 0x0AB6)

Peripheral: 0x4006A000 - 0x4006A020 (permissions: 0x0AB6)

Peripheral: 0x40066000 - 0x4006600C (permissions: 0x0AB6)

Peripheral: 0x4002C000 - 0x4002C08C (permissions: 0x0AB6)
vmpu_enumerate_boxes [DONE]
page heap: [0x20000000, 0x2000a000] 40kB -> 5 8kB pages
uvisor initialized

Debug box becomes initialized right before "vmpu_enumerate_boxes [DONE]":
...
g_debug_box.initialized = 1;
}

/* Load box 0. */ context_switch_in(CONTEXT_SWITCH_UNBOUND_FIRST, 0, 0, 0); DPRINTF("vmpu_enumerate_boxes [DONE]\n");

However interrupts are still disabled at this point which is problematic for deprivilege/reprivilege flow.
The next two lines are still printed with the interrupts disabled, therefore deprivileging isn't performed for them.

We currently share buffers for semihosting and debug box. If we made the two buffers separate, could we cache the debug box buffer output until the debug box is usable? That way, we could get these early prints. Not sure if we want two buffers, though. I'm open to other ideas to get these early prints.

I'll think about the best way to do it.

Patater · 2017-08-31T13:21:06Z

        ".align 4\n"                                // the jump table must be aligned
    "jump_table_unpriv:\n"
        ".word  virq_gateway_out\n"
+#ifdef NDEBUG


I don't like this. We should have the same SVC interface regardless of debug or release build.

Removed the conditional compilation.

Patater · 2017-08-31T13:58:43Z

 }

+/* Jump to unprivileged thread mode, the stack frame is already prepared on PSP. */
+void UVISOR_NAKED virq_gateway_deprivilege(uint32_t svc_sp, uint32_t svc_pc)


This has nothing to do with virq_gateway. Could we give it a better name, please?

Renamed deprivilege and return functions to:
debug_uvisor_deprivilege
debug_uvisor_return
and moved them to debug_box.c

Patater · 2017-08-31T14:01:31Z

-    /* Set the priority of each exception. SVC is lower priority than
-     * MemManage, BusFault, UsageFault, and SecureFault_IRQn, so that we can
-     * recover from security violations more simply. */
+    /* Set the priority of each exception. SVC is set to higher priority than


Please change the priority of exceptions in its own commit, so we can test that changing the priority doesn't cause us to fail tests, independently of other changes you've made.

Made it a separate commit in this PR.

Changed the design to reset active bits, so reverted this change.

Patater · 2017-08-31T14:02:27Z

    }

+    /* If we have a Hard Fault due to the Bus or MemManage Fault escalation we'll try to treat
+     * it as the original fault. */


The handling of escalated hard faults should also be in its own commit, so we can test it independently of other changes.

What happens if we hard fault a second time as a result of trying to handle the escalated fault? Do we enter a hard fault forever loop? Such a situation seems not so great.

Made it a separate commit in this PR.

Still need to think whether we can stuck in a Hard Fault loop.

Changed the design to reset active bits, so reverted this change.

Patater · 2017-08-31T14:03:13Z

+    	if (VMPU_SCB_BFSR) {
+    		ipsr = BusFault_IRQn;
+    	} else
+    	if (VMPU_SCB_MMFSR) {


Please follow style guidelines. else and if should be on same line here.

Patater · 2017-08-31T14:23:44Z

+{
+    /* Return to handler mode with MSP using a basic stack frame. */
+    asm volatile(
+        "ldr lr, =0xfffffff1\n"


Would anything bad happen if another uVisor SVC was called before thevirq_gateway_return SVC?

What would happen to MSP?

Are we taking any steps to prevent calls to other SVC while in the debug box?

Can the virq_gateway_return SVC be used to leak information stored on the MSP to its caller?

It's possible to execute SVC during deprivileging since deprivileged code operates at lower exception level.
No information is supposed to leak during return - we're using the previously created on MSP stack frame.

romkuz01 · 2017-09-10T12:19:28Z

retest uvisor

Changes: - Add a debug_deprivilege_and_return() which performs deprivilege and return within the function itself - Add debug_print() handler to TUvisorDebugDriver structure - Add two SVC handlers to assist deprivilege and return flow

romkuz01 · 2017-09-11T12:41:34Z

@mikisch81 @orenc17A @theamirocohen
Please review

Patater

Should we consider using a separate stack for the debug context, so that we don't overwrite any used state? For instance, when an interrupt is being actively serviced by a box that is also acting as the debug box, we might overwrite the oldest stack data, preventing exit from the interrupt from working correctly after having done a debug print.

Patater · 2017-09-18T16:50:41Z

Merged debug box printing to dev/romkuz01/debug-box-print-experimental due to high risk change and lack of thorough testing

Merged ready to go commits to master

alzix suggested changes Aug 29, 2017

View reviewed changes

romkuz01 added 3 commits August 31, 2017 00:06

Rename debug_deprivilege_and_return() to debug_deprivilege_and_die()

1fdf375

This better reflects the nature of this function. The real debug_deprivilege_and_return() will be added later.

Don't copy xPSR from the source exception stack frame

4344853

Since we're not switching back to the same point this copying makes no sense. Moreover, it can create problematic behavior if ICI/IT field is set to non-zero state on the target stack frame during the copying.

romkuz01 force-pushed the romkuz01_dev_deprivilege_and_return branch from c8ee9f3 to ea32d0f Compare August 31, 2017 11:40

Patater suggested changes Aug 31, 2017

View reviewed changes

Patater reviewed Aug 31, 2017

View reviewed changes

romkuz01 force-pushed the romkuz01_dev_deprivilege_and_return branch from ea32d0f to 3b69672 Compare September 4, 2017 08:55

Patater mentioned this pull request Sep 6, 2017

Handle armv8m unpriv_access with SAU #486

Open

romkuz01 force-pushed the romkuz01_dev_deprivilege_and_return branch from 3b69672 to c112576 Compare September 7, 2017 14:30

romkuz01 force-pushed the romkuz01_dev_deprivilege_and_return branch from e4c52f8 to e5f6c44 Compare September 11, 2017 07:03

romkuz01 added 2 commits September 11, 2017 01:13

Add deprivilege/print/reprivilege flow for ARMv7-M

0eeba6d

Changes: - Add a debug_deprivilege_and_return() which performs deprivilege and return within the function itself - Add debug_print() handler to TUvisorDebugDriver structure - Add two SVC handlers to assist deprivilege and return flow

Use tests from PR 150

d452560

romkuz01 force-pushed the romkuz01_dev_deprivilege_and_return branch from e5f6c44 to d452560 Compare September 11, 2017 08:14

Patater reviewed Sep 12, 2017

View reviewed changes

Patater closed this Sep 18, 2017

Conversation

romkuz01 commented Aug 29, 2017

Uh oh!

alzix left a comment

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 left a comment

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!

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!

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!

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 Aug 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

Patater Aug 31, 2017 •

edited

Loading