Infinite loop in disassembler when processing nested delay slot

[pkooiman]

This is a pathological case but I came across it in a real-world framework. In this case it is MIPS but I think it affects all processors that have delay slots. The code in question is something like (object file attached as well):

   0:   24050040        li      a1,64
   4:   24030006        li      v1,6
   8:   90820000        lbu     v0,0(a0)
   c:   30420040        andi    v0,v0,0x40
  10:   14430004        bne     v0,v1,24 <infiniteloop+0x24>
  14:   00000000        nop
  18:   1000ffff        b       18 <infiniteloop+0x18>
  1c:   1000fffa        b       8 <infiniteloop+0x8>
  20:   00000000        nop
  24:   1445fff8        bne     v0,a1,8 <infiniteloop+0x8>
  28:   00001021        move    v0,zero
  2c:   03e00008        jr      ra
  30:   00000000        nop

The instructions at addresses 18 and 1c will send the disassembler into an infinite loop that eventually causes an out of memory exception. While the branch at address 1C is definitely invalid, I guess it should not lead to a "DoS" and as I said this is found in a real-world application.

The idea seems to be that address 18 should never be reached, hence the tight loop, and there is a NOP missing before the next branch instruction.

The problem happens in Framework/SoftwareModeling/src/main/java/ghidra/program/disassemble/Disassembler.java, in function processInstruction:

protected Address processInstruction(PseudoInstruction inst, MemBuffer blockMemBuffer,
			InstructionBlock block, InstructionSet instructionSet)
			throws InsufficientBytesException, UnknownInstructionException,
			AddressOverflowException, NestedDelaySlotException {
		
		if (followFlow) {
			processInstructionFlows(inst, block);
		}

		List<PseudoInstruction> delaySlotList = parseDelaySlots(inst, blockMemBuffer, block);

		block.addInstruction(inst);

Here, first processInstructionFlows is called for the block starting at address 18, as this is a branch to itself the call will add address 18 again to the disassemblerQueue for later analysis. Then, parseDelaySlots is called and throws a NestedDelaySlotException when it sees the branch at address 1c. The exception is caught in disassembleInstructionBlock, where the next loop iteration finds the same address 18 on the disassemblerQueue; and because the instruction was not yet added to the block, the check to see if the block has already been disassembled fails and we have an infinite loop.

A very quick fix is to move block.addInstruction(inst) before the call to parseDelaySlots but I am not sure that will not cause other problems.

I attach a small object file that shows this behavior when you try to disassemble or auto-analyze it.
nesteddelayslot.zip

[GhidorahRex]

This is definitely a pathological case. We can fix it so that it will not cause the infinite loop, but it still won't process the instructions because of the branch in the delay slot. This situation is considered "UNPREDICTABLE" prior to release 6, and generate a Reserve Instruction Exception in release 6.

How was the code generated in the object file? I would assume that the assembler would disallow this situation.

[pkooiman]

Absolutely, fixing it so the disassembler does not choke is all I was hoping for!
I can only guess how the original firmware I am working on ended up having this filthy construct.. I generated the obj file using

#define WTF()		asm volatile(".word	0x1000ffff")

int infiniteloop(unsigned long iobase)
{
    unsigned char status,c_status;
    unsigned long count = 0;
    
    c_status = 0x40; 
          
    while(1)
    {
          status = *(volatile unsigned char*)(iobase) & 0x40;
    	  
        count++;
        if(status == 0x06) {
        		WTF();
        }
        else if(status == c_status)
            return 0;
    }
    
    return 1;
}

which best I can tell is the intention of the original code, ie to hang (and possibly provoke and exception) if the forbidden situation where status==6 occurs. The branch within the delay slot is inserted by the compiler for the while() loop.

[ghidra1]

Fix implemented for 9.1.2