Interrupt hole in VProtect

While examining a stack dump I spotted the following code which VProtect copies onto the stack and uses to RMLoad/RMRun modules:

MOV     R0,#&0A
MOV     R1,R7
SWI     XOS_Module
BVS     error
CMP     R3,#0
MOVEQ   R0,#2
MOVEQ   R2,R5
MOVEQ   R1,#&7800
SWIEQ   XOS_Module
.error
MOV     R13,R4
STRVS   R0,[R13,#0]
LDMIA   R13!,{R0-R7,PC}

Admittedly the chances of a receiving an interrupt during those last two instructions is pretty slim, and (as far as I know) I’ve never encountered a crash due to this bug, but I figured it was worth mentioning now that I’ve spotted it.

I’m going to put my hand up and ask to be educated! Can anyone explain in more detail why this is unsafe? I don’t quite follow.

Using a fictional stack starting at &100, the stack would look something like this once the code has been copied onto it:

&AC MOV     R0,#&0A
&B0 MOV     R1,R7
&B4 SWI     XOS_Module
&B8 BVS     &D0
&BC CMP     R3,#0
&C0 MOVEQ   R0,#2
&C4 MOVEQ   R2,R5
&C8 MOVEQ   R1,#&7800
&CC SWIEQ   XOS_Module
&D0 MOV     R13,R4
&D4 STRVS   R0,[R13,#0]
&D8 LDMIA   R13!,{R0-R7,PC}
&DC (stacked R0)
&E0 (stacked R1)
&E4 (stacked R2)
&E8 (stacked R3)
&EC (stacked R4)
&F0 (stacked R5)
&F4 (stacked R6)
&F8 (stacked R7)
&FC (stacked PC)

On entry to the code, R13 will be &AC, and R4 will be &DC. Up until &D0, R13 is <= PC, so the code which is on the stack and currently being executed is safe from being overwritten. But once the instruction at &D0 has been executed R13 will have been updated to &DC, leaving the not-yet-executed code at &D4 and &D8 in danger of being overwritten if anything else uses the SVC stack during that time.

Thanks, Jeffrey. That makes a bit more sense. Is this code already running in SVC mode? So R13 is pointing at the SVC stack? If so, I think I get it. What would cause an interrupt routine to start using the SVC stack rather than the IRQ stack?

Is this code already running in SVC mode?

Yes.

What would cause an interrupt routine to start using the SVC stack rather than the IRQ stack?

Calling a SWI would be the main (only?) cause.

Ah, beat me to it!

There are two main reasons for switching to SVC mode in an interrupt handler: the main one is if you want to call a SWI. Otherwise, there’s a significant danger that some SVC mode code was active when the interrupt fired, and the foreground’s R14_svc will be corrupted if the interrupt handler doesn’t switch to SVC mode and stack R14 before calling a SWI.

The other one is if you want to re-enable interrupts within an interrupt handler. One effect this has is that you can no longer use BL because R14_irq can be corrupted at any time. It’s possible to work around this by using (for example) MOV R12,PC then B subroutine, but notably no C compilers (to my knowledge) have ever been able to generate such code. For these two reasons, any interrupt handler written in C will run in SVC mode – CMHG generates all the necessary assembler magic to make the switch and back again.

The other one is if you want to re-enable interrupts within an interrupt handler.

Yes, of course. I’m not sure how I forgot about that one.

Thanks, guys – much appreciated. So what’s the solution? Disable interrupts before the MOV R13,R4? Or use another register (R8?) to copy R4 into before popping R0-R7, only updating R13 to recover the space used on the stack as the very last instruction that affects the return?

Disable interrupts before the MOV R13,R4?

Disabling interrupts would be one solution, but that would complicate things because after restoring the registers you’d have to find a way to return from the stack and re-enable IRQs in just one instruction. This would probably end up being MOVS PC,R14, with R14/SPSR set up appropriately (hence complicating things, since – if the code is written for 26 & 32bit targets, you’d need a different approach for each)

Or use another register (R8?) to copy R4 into before popping R0-R7, only updating R13 to recover the space used on the stack as the very last instruction that affects the return?

Using another register to store R8 wouldn’t help, since that would be one more register that needs restoring on exit.

There are a couple of easy solutions I can think of. Either have the desired R13 value (i.e. &100) stored as part of the stack frame, so you can do this:

STRVS R0,[R4,#0]
LDMIA R4,{R0-R7,R13,PC}

Or rearrange the stack frame so that the PC value is stored below the copied code, with R13 pointing at it, so you can do this:

STRVS R0,[R4,#0]
LDMIA R4,{R0-R7}
LDR PC,[R13],#xxx

The first approach is perhaps the easiest, as it avoids you needing to calculate the right value of ‘xxx’. Unfortunately ARM have deprecated LDM/STM with R13 in the register list, so it might stop working at some point (ARMv8?). But I wouldn’t worry about it too much, since the standard way of returning from an APCS function is to pop the stack frame (including R13) from the stack using a single LDM – so if ARM do decide to break LDM {…R13…} we’ll lose binary compatability with all existing C programs. Compared to that, VProtect not working would just be a minor issue!

But I wouldn’t worry about it too much, since the standard way of returning from an APCS function is to pop the stack frame (including R13) from the stack using a single LDM – so if ARM do decide to break LDM {…R13…} we’ll lose binary compatability with all existing C programs.

This is painfully obvious every time you assemble any code written to use the frame pointer with the latest version of objasm, of course. I was tempted to disable the warning about that particular deprecated instruction because of that, but in the end I came down on the side of leaving it in. At least that means more people will be aware of that potential compatibility timebomb that ARM have stored up for us.

I really hope that it’s just ARM’s way of pointing out that there’s no equivalent encoding for that instruction in Thumb-2 code, and so any attempt to reassemble the code for a CPU that only has Thumb-2 but not ARM instruction set support will fail. Of course, trying to make RISC OS support such CPUs would be an even bigger challenge again than" just" losing binary compatibility will all existing C programs!

At least that means more people will be aware of that potential compatibility timebomb that ARM have stored up for us.

Any particular reason why ARM have taken the step of deprecating R13? I suppose they could use the register bit as some sort of flag, but it would risk screwing up processor compatibility with older code, leading to the potential of a pile of quirky hard to find bugs (a little like the rotated loads issue).
Can somebody point me to the doc at arm.com that mentions this? There might be more info buried in there…

It’s in the ARMv7-AR ARM. I don’t think they’ve given any justification for it, but it wouldn’t surprise me if it’s part of their general movement to make R13 a special register instead of a general-purpose one (IIRC in issue B of the ARMv7 ARM they tried deprecating the use of R13 in several other ways, until someone managed to talk them out of it and they went back on it in issue C).

Ah, it looks like they are finally codifying R13 as the Stack Pointer [p45 of 2734!].
That isn’t a surprise; so long as they don’t screw around with the instruction set in a half-assed attempt to enforce this (for sometimes you need to do some unpleasant stuff with SP and LR in re-entrant code).

Argh! They refer to them as a register file ! I first saw this use in a PIC documentation and I hated it then. Why can’t registers just be registers? There’s no “file” about it. I can’t read or write data from a block or stream, there’s no FIFO, it’s just a bunch of registers. IT IS NOT A FILE! Grrrr-argh!

Just realised I haven’t thanked you guys for taking the time to explain this. All clear now, though I’m not sure I’d be astute enough to notice the problem in the first place. I guess “running code on stack” = “watch out!”