x86: Incorrect LEAVE instruction?

[mlgiraud]

Hi, I stumbled across an (imo) incorrect definition of the LEAVE instruction for x86: https://github.com/NationalSecurityAgency/ghidra/blob/7148590e5c4d917ea4b0add3507c540a54b771d4/Ghidra/Processors/x86/data/languages/ia.sinc#L3332 The intel manual specifies:

IF StackAddressSize = 32
THEN
ESP := EBP;
ELSE IF StackAddressSize = 64
THEN RSP := RBP; FI;
ELSE IF StackAddressSize = 16
THEN SP := BP; FI;
FI;
IF OperandSize = 32
THEN EBP := Pop();
ELSE IF OperandSize = 64
THEN RBP := Pop(); FI;
ELSE IF OperandSize = 16
THEN BP := Pop(); FI;
FI;

So if I understand this correctly, then in the linked LEAVE instruction, the semantic section should instead pop to BP, and not EBP, since opsize is 16 (0) and addrsize is 32 (1). Since this spec seems to be parsed correctly by the sleigh compiler, i also assume that there is a bug in the size checking, since pop42 is defined as (https://github.com/NationalSecurityAgency/ghidra/blob/7148590e5c4d917ea4b0add3507c540a54b771d4/Ghidra/Processors/x86/data/languages/ia.sinc#L1585)

macro pop42(x) {
  x = *:2 $(STACKPTR);
  ESP = ESP + 2;
}

This should resolve to EBP = *:2 $(STACKPTR) which would imply that the compiler implicitly zero-extends the value from two bytes to four bytes.

If i misunderstood anything, then please let me know.

[mlgiraud]

There is a similar issue in th IRETQ instruction. It is currently defined as:

:IRETQ          is $(LONGMODE_ON) & vexMode=0 & addrsize=2 & opsize=2 & byte=0xcf            { pop88(RIP); local tmp:8=0; pop88(tmp); CS=tmp(0); pop88(eflags); return [RIP]; }

However, this should probably call pop88(rflags). popping an 8 byte register into a four byte regsiter makes no sens imo.

Another one:

:RETF           is $(LONGMODE_ON) & vexMode=0 & addrsize=2 & opsize=1 & byte=0xcb            { pop48(EIP); RIP=zext(EIP); local tmp:4=0; pop44(tmp); CS=tmp(0); return [RIP]; }

I believe this should be

:RETF           is $(LONGMODE_ON) & vexMode=0 & addrsize=2 & opsize=1 & byte=0xcb            { pop84(EIP); RIP=zext(EIP); local tmp:4=0; pop84(tmp); CS=tmp(0); return [RIP]; }

but i haven't had time to cross-check this with the manual.

[GhidorahRex]

I believe you're correct on ENTER and LEAVE, as well as the IRETQ, but I think the RETF line is correct. However, I htink the RETF on line 4006 is wrong:

:RETF imm16     is $(LONGMODE_ON) & vexMode=0 & addrsize=2 & opsize=1 & byte=0xca; imm16     { pop44(EIP); tmp:4=0; pop44(tmp); RIP=zext(EIP); CS=tmp(0); RSP=RSP+imm16; return [RIP]; }

should be:

:RETF imm16     is $(LONGMODE_ON) & vexMode=0 & addrsize=2 & opsize=1 & byte=0xca; imm16     { pop44(EIP); tmp:4=0; pop44(tmp); RIP=zext(EIP); CS=tmp(0); RSP=RSP+imm16; return [RIP]; }

Generally, with the popNn macros it's pop<2^opsize><2^addrsize> but there's various exceptions for specific instructions, and we don't perfectly model the opsize and addrsize (or model stack address size at ALL).

[mlgiraud]

Are you sure about pop<2^opsize><2^addrsize>? I think its the other way round. the pop48(EIP) tries to load an 8 byte value into a 4 byte register:

macro pop48(x) {
  x = *:8 $(STACKPTR);
  ESP = ESP + 8;
}

should expand to

EIP = *:8$(STACKPTR);
ESP = ESP + 8;

which does not make any sense to me as we would be implicitly discarding half of the register. Also, the instruction matches on opsize=1, meaning we are dealing with 32 bit operands if i understand the spec correctly, hence it does not make sense to me to try and pop an 8 byte operand.

EDIT: I fixed a these issues in my own branch and also added the local keyword everywhere where it was not enforced by the Ghidra sleigh compiler. If you like i can create a pull request for this.

EDIT2: I believe this also is related to #6856 since these bugs were all detected with a more strict size inference/checking. The sleigh compiler seems to simply ignore sizes on loads, i.e. it seems to be possible to load an 8 byte value and stuff it into a 4 byte varnode, and the other way round.

[mlgiraud]

In agree that there seem to be more issues in that part, but i believe the quted macro is correct. It does not make any sense imho to load a 4 byte value and then reduce the stack by 8 bytes.

[mlgiraud]

The stack address size determines whether we change RSP, ESP, or SP. The operand size determines what size we load and by what amount we want to reduce or increase the stack depending on whether we are poping or pushing. And the current usage to me suggests, that the first number is the address size, and the second the operand size

[Wall-AF]

In agree that there seem to be more issues in that part, but i believe the quted macro is correct. It does not make any sense imho to load a 4 byte value and then reduce the stack by 8 bytes.

I dropped my comment when I realised I'd made a mistake! (Too slow, obviously!)