ish-app / ish

Linux shell for iOS
https://ish.app
Other
17.24k stars 910 forks source link

Incorrect execution of instructions #511

Closed tommythorn closed 5 years ago

tommythorn commented 5 years ago

This is a funny one. The attached instruction prints 'Got 4095' whereas on a real i686 it prints -1. I'm fairly sure the issue is in the execution of

 8048e95:       0f b7 44 24 1e          movzwl 0x1e(%esp),%eax
 8048e9a:       66 c1 f8 04             sar    $0x4,%ax
 8048e9e:       98                      cwtl   

but I'm not equipped to debug it further. bug.zip

saagarjha commented 5 years ago

Just those three instructions

08049000 <_start>:
 8049000:       b8 f0 ff 00 00          mov    $0xfff0,%eax
 8049005:       66 c1 f8 04             sar    $0x4,%ax
 8049009:       98                      cwtl
 804900a:       b8 01 00 00 00          mov    $0x1,%eax
 804900f:       cd 80                   int    $0x80

seem to be fine:

EIP = 0x08049000 EAX = 0x00000000
EIP = 0x08049005 EAX = 0x0000fff0
EIP = 0x08049009 EAX = 0x0000ffff
EIP = 0x0804900a EAX = 0xffffffff

Something else must be going on in your binary…let me try take a closer look at that.

Nevermind, this works correctly on my x86 system but not on ARM. It looks like sar is not right:

EIP = 0x08049000 EAX = 0x00000000
EIP = 0x08049005 EAX = 0x0000fff0
EIP = 0x08049009 EAX = 0x00000fff
EIP = 0x0804900a EAX = 0x00000fff
saagarjha commented 5 years ago

@tbodt is it legal to use sar here? Won't it act on the 32-bit register and copy the leading zero instead of bit 16? Could we instead shift the value up all the way so that its leading bit is flush with the register MSB, apply the sar, then shift back?

tommythorn commented 5 years ago

I should have pointed out that this is the output of gcc operating on bitfields. I don’t have the source here but it’s essentially (typing on a phone)


typedef unsigned opcode_t;

typedef union riscv_instruction {
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   rd       :  5;
        unsigned   funct3   :  3;
        unsigned   rs1      :  5;
        unsigned   rs2      :  5;
        unsigned   funct7   :  7;
    } r;
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   rd       :  5;
        unsigned   funct3   :  3;
        unsigned   rs1      :  5;
        int        imm11_0  : 12;
    } i;
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   imm4_0   :  5;
        unsigned   funct3   :  3;
        unsigned   rs1      :  5;
        unsigned   rs2      :  5;
        int        imm11_5  :  7;
    } s;
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   imm11    :  1;
        unsigned   imm4_1   :  4;
        unsigned   funct3   :  3;
        unsigned   rs1      :  5;
        unsigned   rs2      :  5;
        unsigned   imm10_5  :  6;
        int        imm12    :  1;
    } sb;
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   rd       :  5;
        int        imm31_12 : 20;
    } u;
    struct {
        opcode_t   opext    :  2;
        opcode_t   opcode   :  5;
        unsigned   rd       :  5;
        unsigned   imm19_12 :  8;
        unsigned   imm11    :  1;
        unsigned   imm10_1  : 10;
        int        imm20    :  1;
    } uj;
    u_int32_t raw;
} insn_t;

Insn_t i = { .raw = -1 }.
Printf(“%d”, i.i.i imm11_0);

and compiled without optimization.

tbodt commented 5 years ago

Looks like there needs to be an sxth at the start of that sar block.

Also congrats @tommythorn on finding the last known bug in the emulator. Let me know where I should send your $2.56. :D