Issues with ESIL flag computation

[sushant94]

There are some issues when esil computes flags. This bug is to consolidate these and work towards a fix.

• [ ] ESIL never sets Adjust Flag (AF)
• [ ] Set bit 1 of eflags to be 1 always
• [x] #3233
• [ ] Fix other x86 related reg profiles too

Completed:

• [x] Carry flag for subtraction is set using $c,cf,= rather than using $b,cf,=
• [x] Overflow flag is not computed correctly (for subtraction)
• [x] x86 add (+=) does not set flags.
• [x] x86 sub does not set pf
• [x] Parity flag is not computed correctly.
• [x] Incorrect register profile for x86 (eflags is not set to correct value even when flags are set correctly)

I'm working towards a fix (and respective tests). Will update this issue when I find more bugs.

TODO(Additions):

• [x] Implement ROL/ROR/RCR/RCL
• [ ] Carry flag for SHL/SHR/SAL/SAR
• [x] Instructions like ADC/SBB etc.

---

EDIT: Checked the ones that I fixed EDIT2 (29.10.15): Added more issues EDIT2 (29.10.15): Added TODO EDIT3(30.10.15): Added more issues EDIT4(27.3.16): Reordered for readability

[sushant94]

Some more points to consider:

• How do we go about setting the CF for SHL/SHR (and other variants) [See Link]
• Even though new operations >>> and <<< are in ESIL, these are not currently used to implement ROR/ROL of x86. It is possible to implement these using this, however, the other variants (RCL/RCR) include the carry flag in the shifts and hence we cannot use this directly. [See Link]

[condret]

$b,cf,= will never work, it must be $b63,cf,= for example

[condret]

the shl/shr thing is easy. it works similiar in gb-land: srl a 1,a,&,C,=,1,a,>>=,$z,Z,=,0,N,=,0,H

[condret]

i really don't like >>> and <<< because they need 3 args pushed on the stack, if i remember right

[sushant94]

@condret Why do you say just $b dies not work? Cause this seems to be working. Then again, I might be wrong and it might have just worked for this case.

I don't know about gb, but for x86, the carry flag is not defined if the number of bits shifted out is more than the width of the destination. I'm not sure on how to account for this.

And I don't know where >>> and <<< are used and who introduced it. It could be used here but we need to find a way around for RCL/RCR.

[XVilka]

Added the 0.10 milestone, since I think ESIL is now widely (more or less) used, so it is better to provide fixes in the release.

[condret]

@sushant94 $b means borrow-from-bit, if you just do $b, you do not define from what bit you want to check the borrow. So $b performs undefined behaviour, which may end up in analdeamons eating your pet/child/keyboard, or launching nethack. rlc/rcr can be done easily with DUP operation, or use $cx how i did it for gb: rlc a 1,a,<<=,$c7,C,=,C,a,|=,$z,Z,=,0,H,=,0,N,

[radare]

Can we move this for 1.0, i dont think this is blocking for 0.10 release. it is "only" needed for radeco, which there's no release planed yet.

[sushant94]

In any case, this issue is almost fixed (atleast the points highlighted here).

[safiire]

I've spent some time trying to track down why parity bit for asm sequence always becomes 1:

mov edi, 2 
or edi, edi

When the ESIL for the or is evaluated we have:

rdi,rdi,|=,
0,of,=,
0,cf,=,
$s,sf,=,
$z,zf,=,
$p,pf,=

Parity is calculated on esil->cur, which is expected to still be 2, however by the time we get to $p,pf,=, esil->cur has been set to 0 by flag update instructions such as 0,of,=. This causes the parity function to always compute the parity(0) = 1 in this case.

I seem to see an attempt to prevent the flag updates from altering esil->cur here, when 0,of,= runs esil_eq(...) in esil.c.

 if (ret && r_anal_esil_get_parm_type (esil, src) != R_ANAL_ESIL_PARM_INTERNAL) { //necessary for some flag-things
  esil->cur = num2;
  esil->old = num;
 }

What this is doing is checking if src is R_ANAL_ESIL_PARM_INTERNAL, ie begins with $, but doesn't work in the case of 0,of,= as the type of src and dst are both R_ANAL_ESIL_PARM_NUM, and R_ANAL_ESIL_PARM_REG respectively. So the conditional guard fails and esil->cur gets updated to 0, and that is what we calculate the parity of afterwards.

Flags such as of are considered R_ANAL_ESIL_PARM_REG, and I don't immediately see any code that can make a distinction between flags and registers. It seems like we want flag updates to avoid updating esil->cur so my first idea was to decorate flag registers with some symbol, in the same way $p is, maybe something like $of, and then check if either src or dst is R_ANAL_ESIL_PARM_INTERNAL and skip updating esil->cur.

What would that look like:

rdi,rdi,|=,
0,$of,=,
0,$cf,=,
$s,$sf,=,
$z,$zf,=,
$p,$pf,=

And the guard around updating esil->cur would look like:

if (ret && (r_anal_esil_get_parm_type (esil, src) != R_ANAL_ESIL_PARM_INTERNAL ||
           (r_anal_esil_get_parm_type (esil, dst) != R_ANAL_ESIL_PARM_INTERNAL)) {
  esil->cur = num2;
  esil->old = num;
 }

I've only been looking into radare2's source for about a week, so it's quite possible I am not seeing some bigger issue that altering flags like of to be $of so they are considered internal instead of regs. In any case I'm really interested in contributing to this project, so I'd love to hear what you think. I also assume this affects other flag updates, as the same if() guard is applied to many other esil_{op} functions.

The code I've been testing against is the following, that could go into a test.

push rbp
mov rbp, rsp
mov rdi, 2
or rdi, rdi
pushfq
pop rax
shr rax, 2
xor rbx, rbx
inc rbx
and rax, rbx
pop rbp
ret

Where parity of edi, 2 here, should return 0 in rax.

[safiire]

One other fix I had made in libr/anal/p/anal_x86_cs.c, was in the ESIL generated for opcodes in the pushf family, which originally looks like this:

case X86_INS_PUSH:
case X86_INS_PUSHF:
case X86_INS_PUSHFD:
case X86_INS_PUSHFQ:
    {
        char *dst = getarg (&gop, 0, 0, NULL);
        esilprintf (op, "%d,%s,-=,%s,%s,=[%d]",
            rs, sp, dst?dst:"eax", sp, rs);
        free (dst);
    }
    break;

Which I've updated to:

case X86_INS_PUSH:
{
  char *dst = getarg (&gop, 0, 0, NULL);
  esilprintf (op, "%d,%s,-=,%s,%s,=[%d]",
  rs, sp, dst?dst:"eax", sp, rs);
  free (dst);
}
break;
case X86_INS_PUSHF:
{
  esilprintf (op, "%d,%s,-=,flags,%s,=[%d]",
  rs, sp, sp, rs);
}
break;
case X86_INS_PUSHFD:
case X86_INS_PUSHFQ:
{
  esilprintf (op, "%d,%s,-=,%cflags,0xfcffff,&,rsp,=[%d]",
  rs, sp, rs==8?'r':'e', rs);
}
break;

Which according to this page: http://x86.renejeschke.de/html/file_module_x86_id_271.html says:

When copying the entire [R | E]FLAGS register to the stack, the VM and RF flags (bits 16 and 17) are not copied; instead, the values for these flags are cleared, so I & with 0xfcffff.

I can make a pull request for this if you like.

[sushant94]

@safiire Your first issue of X86_INS_OR not correctly setting the flag due to esil->cur and esil->old being modified from clearing of of and cf can be fixed by simply modifying the order of operations to perform the clearing at the end. True that this maybe a hacky fix and we need a more permanent solution, but this works for now (I'll make a PR for this fix soon).

Please do not add '$' to the flag variables like you have done so. Sure it may work for emulation purposes, but some static analysis built around esil will break as '$' indicates an internal variable which has a fixed meaning :)

[radare]

this issue is almost finished, could you guys test and check the pending things? thx