Skip to content

Commit ba9b79c

Browse files
committed
Fix for #62: don't rely on zero_stack setting \base to \top.
The zero_stack macro in the switcher did not meet the claim in the comment that the base register would end up pointing to the top (it was not true if the top was not 32-byte aligned). Rather than fix this I've removed the claim in the comment and adjusted the places that call it not rely on this property.
1 parent 361695a commit ba9b79c

File tree

3 files changed

+34
-39
lines changed

3 files changed

+34
-39
lines changed

sdk/core/loader/boot.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,7 @@ namespace
811811
Debug::log("New thread's trusted stack is {}", threadTStack);
812812
threadTStack->mepcc = pcc;
813813
threadTStack->cgp = cgp;
814-
auto stack = allocateStack(config.stackSize);
814+
auto stack = allocateStack(config.stackSize);
815815
threadTStack->csp = stack;
816816
Debug::log("New thread's stack is {}", stack);
817817
// Enable previous level interrupts and set the previous exception
@@ -820,7 +820,7 @@ namespace
820820
(priv::MSTATUS_MPIE |
821821
(priv::MSTATUS_PRV_M << priv::MSTATUS_MPP_SHIFT));
822822
#ifdef CONFIG_MSLWM
823-
threadTStack->mslwm = stack.top();
823+
threadTStack->mslwm = stack.top();
824824
threadTStack->mslwmb = stack.base();
825825
#endif
826826
threadTStack->frameoffset = offsetof(TrustedStack, frames[1]);

sdk/core/switcher/entry.S

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,10 @@ switcher_scheduler_entry_csp:
105105
.endm
106106

107107
/**
108-
* Zero the stack. The three operands are the base address (modified during
109-
* this call, will point at the top at the end), the top address, and a scratch
110-
* register to use. The base must be a capability but it must be provided
111-
* without the c prefix because it is used as both a capability and integer
112-
* register. Top and scratch are both clobbered.
108+
* Zero the stack. The three operands are the base address, the top address,
109+
* and a scratch register to use. The base must be a capability but it must
110+
* be provided without the c prefix because it is used as both a capability
111+
* and integer register. All three registers are clobbered.
113112
*/
114113
.macro zero_stack base top scratch
115114
addi \scratch, \top, -32
@@ -181,24 +180,20 @@ compartment_switcher_entry:
181180
cgetbase s1, csp
182181
csetaddr csp, csp, s1
183182
sub s1, s0, s1
184-
csetboundsexact csp, csp, s1
183+
csetboundsexact ct2, csp, s1
184+
csetaddr csp, ct2, s0
185185
#ifdef CONFIG_MSLWM
186186
// read and align the stack low water mark
187187
csrr gp, 0xbc1 // mslwm
188188
and gp, gp, ~0xf
189+
// skip zeroing if low water mark >= stack poitner
190+
bge t2, sp, after_zero
189191
// use stack low water mark as base address for zeroing
190192
// XXX could be out of bounds / unrepresentable if bad csp?
191-
csetaddr csp, csp, gp
192-
// go to zeroing if the stack low water mark is less than current stack pointer
193-
blt sp, s0, zeroing
194-
// otherwise reset address of csp
195-
// the zeroing loop assumes sp <= s0, so make this true
196-
// we want csp to end up pointing at stack top
197-
csetaddr csp, csp, s0
198-
// now fall through to zeroing, which will have nothing to do
193+
csetaddr ct2, csp, gp
199194
#endif
200-
zeroing:
201-
zero_stack sp, s0, gp
195+
zero_stack t2, s0, gp
196+
after_zero:
202197
#ifdef CONFIG_MSLWM
203198
// store new stack top as stack low water mark
204199
csrw 0xbc1, sp // mslwm
@@ -740,7 +735,7 @@ exception_entry_asm:
740735
csetaddr ct2, csp, tp
741736
zero_stack t2, t1, tp
742737
#ifdef CONFIG_MSLWM
743-
csrw 0xbc1, t2 // mslwm
738+
csrw 0xbc1, sp // mslwm
744739
#endif
745740
#endif // CONFIG_NO_SWITCHER_SAFETY
746741
cret

sdk/core/switcher/tstack.h

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -44,24 +44,24 @@ struct TrustedStackFrame
4444
template<size_t NFrames>
4545
struct TrustedStackGeneric
4646
{
47-
void *mepcc;
48-
void *c1;
49-
void *csp;
50-
void *cgp;
51-
void *c4;
52-
void *c5;
53-
void *c6;
54-
void *c7;
55-
void *c8;
56-
void *c9;
57-
void *c10;
58-
void *c11;
59-
void *c12;
60-
void *c13;
61-
void *c14;
62-
void *c15;
63-
size_t mstatus;
64-
size_t mcause;
47+
void *mepcc;
48+
void *c1;
49+
void *csp;
50+
void *cgp;
51+
void *c4;
52+
void *c5;
53+
void *c6;
54+
void *c7;
55+
void *c8;
56+
void *c9;
57+
void *c10;
58+
void *c11;
59+
void *c12;
60+
void *c13;
61+
void *c14;
62+
void *c15;
63+
size_t mstatus;
64+
size_t mcause;
6565
#ifdef CONFIG_MSLWM
6666
uint32_t mslwm;
6767
uint32_t mslwmb;
@@ -74,9 +74,9 @@ struct TrustedStackGeneric
7474
uint8_t inForcedUnwind;
7575
// Padding up to multiple of 16-bytes.
7676
#ifdef CONFIG_MSLWM
77-
#define TRUSTED_STACK_PADDING 13
77+
# define TRUSTED_STACK_PADDING 13
7878
#else
79-
#define TRUSTED_STACK_PADDING 5
79+
# define TRUSTED_STACK_PADDING 5
8080
#endif
8181
uint8_t padding[TRUSTED_STACK_PADDING];
8282
/**

0 commit comments

Comments
 (0)