Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 12 - Retro Bad Code

What's wrong with this code, part 12 - Retro Bad Code

  • Comments 23
I was at a security tech talk last week discussing some fascinating stuff, and it reminded me of an interview question that my manager used to give to people who said that they understood x86 Assembly language.

I realized that it would make an interesting "retro" "what's wrong with this code" so, here goes.

 

When you're writing code for low level platforms (REALLY low level platforms), one of the first things you need to start handling is processor interrupts.  On the x86 family of processors, when an interrupt is generated (either hardware or software), the processor generates a trap frame with the following information (I may have the CS and IP backwards, check your processor manual):

Your code is now executing, but you're running on the application's stack.  So the very first thing that has to happen in your interrupt handler is that you've got to switch to your own stack (you need to do this because you don't know how much memory is remaining on the user stack - if you overflow the user's stack, you've got problems.

So you'd have to write code that switches from the user's stack to your kernel mode stack.

My boss used to use an example that was the system call dispatcher for a theoretical operating system, which maintained a 32bit pointer to the kernel mode stack in a global variable, I'll continue that tradition.


    <code to establish DS>
    MOV    [Saved_SS], SS
    MOV    [Saved_SP], SP
    MOV    SP, WORD PTR [Dos_Stack]
    MOV    SS, WORD PTR [Dos_Stack]+2
    <code to dispatch off of the value in AH>

The problem is that there's a MASSIVE bug in this code.  And it's not because of the use of global variables for the saved SS and SP - assume that the reentrancy issues are handled in the <code to establish DS> section above.

  • A nice easy one then.
    Setting SP before SS, if an interrupt happens at this point then SS:SP points into a an effectively random memory and may well clobber the calling processes stack or data segment.
    The sequence should always be SS then SP which disables interrupts until after the next instruction.
  • Thinking some on this reminds me of when writing some BCD divide and multiply code I snaffled the SS register to allow me to use [di+bp] but have it point to the data segment without having the overhead of the segment override prefixes.
    The code turned off interrupts while I did this and was hand tuned for efficency on a traditional 8086 and is the only code I've ever used xlat in. (which meant bx wasn't available)
  • it overwrites the stack at Dos_Stack.
  • Shouldn't BP also be set to something?
  • As far as I remember (it was a long time ago :) ) if you use SP or BP the default addressing will be SS:SP or SS:BP. So, if I’m right, you will store the stack pointer in the application stack :)

    And, of course, the correct way would be to load SS, and only then SP.
  • No, I am wrong. We do not use SP in addressing here... Should be more attentive...
  • Well, what about 64bit architectures? If we use WORD PTR to get the value from the memory we will end up with only half of the register’s value.
  • Since this is 16 bit code, I'm not worried about what happens when it's run on a 64bit processor...
  • I think the problem will show up if another interrupt (one with a higher priority) happens between the last two MOV instructions like so:

    MOV SP, WORD PTR [Dos_Stack]
    <interrupt here>
    MOV SS, WORD PTR [Dos_Stack]+2

    In that case the stack would get corrupted.

    Cannot wait for the answers ... :)
  • I agree with some of the others, I believe the loads of SP/SS are reversed. From the Intel docs:

    Loading the SS register with a MOV instruction inhibits all interrupts until after the execution
    of the next instruction. This operation allows a stack pointer to be loaded into the ESP register
    with the next instruction (MOV ESP, stack-pointer value) before an interrupt occurs1. Be aware
    that the LSS instruction offers a more efficient method of loading the SS and ESP registers.

    ----

    As SP is loaded first it's possible for an interrupt to occur before SS is loaded (thus the stack is pointing off into who knows where).
  • > <code to establish DS>

    Don’t we also need to save DS somewhere, since we’re going to screw it up by “establishing”?

    Me, I’d probably use my code segment for these critical points where I have no stack and no data segment. And I’d also disable interrupts while mucking around with the stack registers, just in case.

    CLI
    MOV CS:[oldSS], SS
    MOV CS:[oldSP], SP
    MOV SS, CS:[mySS]
    MOV SP, CS:[mySP]
    ; now we have our stack
    ; let’s switch to our data segment as well
    PUSH DS
    MOV DS, CS:[myDS]
    STI
  • I agree with two of the other posters. Setting SP and SS at different times will cause all sorts of problems. Interupts have a magical habit of finding the worst place to happen.
  • Centaur, that doesn't work. Edge got it right.

    The problem is that, even if you use CLI to disable interrupts, there's still the NMI. That interrupt can still happen even when interrupts are disabled.

    Using "LSS SP,..." loads both: SS and SP in one instruction, so the NMI interrupt will happen before either is set, or after both are set. It's the whole purpose for the existence of the LSS instruction.
  • You know what? I'm stoopid. Maybe I should _really_ read the posts, or think a little before jumping the gun.

    I mean... I don't remember if what Edge says (that after executing "MOV SS,..." the interrupts are disabled for one more instruction) was true. It probably was. Also, I don't know if that would apply to NMIs.

    But... there's one more problem. It is reentrancy-related, after all. If an interrupt happens after setting SS and SP, will it use the same stack, thus overwriting the stack of the original interrupt? That can be avoided by using code like this:

    PUSH AX
    MOV AX,SS
    CMP AX,[Dos_Stack_Segment]
    JNE StackChange
    MOV AX,SP
    CMP SP,[Dos_Stack_Begin]
    JB StackChange
    CMP SP,[Dos_Stack_End]
    JB NoStackChange
    StackChange:
    LSS SP,[Dos_Stack]
    NoStackChange:
    POP AX

    Off the top of my head...

    Basically, if the stack already points to the DOS stack, then don't change it. You could also have multiple stacks for the different kinds of interrupts, but that's much more memory-inefficient. As long as your system can somehow limit the amount of interrupt nesting, so that you can size the system stack appropriately, this would work nicely.
  • Then again, Larry did mention that reentrancy was somehow handled already. Ok, I shut up now.
Page 1 of 2 (23 items) 12