Open nshp opened 3 years ago
nshp
commented
3 years ago @yrp604 mentioned a similar issue, #2250. The common point seems to be that these functions change the stack pointer, because they are not real functions - they are more like a common tail to other functions.
plafosse
commented
3 years ago I minimized this a bit and the case and its even weirder...
mr r1, r2
blr assemble this and it clobbers all these:
['ctr', 'f0', 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', 'f11', 'f12', 'f13', 'lr', 'r0', 'r2', 'r3', 'r4', 'r5', 'r6', 'r7', 'r8', 'r9', 'r10', 'r12']LLIL is just this:
0 @ 00000000 r1 = r2
1 @ 00000004 <return> jump(lr)Little bit more info:
mr r0, r2
blr works as expected
>>> current_function.clobbered_regs
['lr', 'r0']mr r1, r2
blr is incorrect:
>>> current_function.clobbered_regs
['ctr', 'f0', 'f1', 'f2', 'f3', 'f4', 'f5', 'f6', 'f7', 'f8', 'f9', 'f10', 'f11', 'f12', 'f13', 'lr', 'r0', 'r2', 'r3', 'r4', 'r5', 'r6', 'r7', 'r8', 'r9', 'r10', 'r12']I think the issue here is something to do with the function not conforming to the calling convention. Funny enough only r1 isn't in the list.... maybe we have some inverted logic 🤔
yrp604
commented
2 years ago Here's another repro:
>>> current_function
<func: aarch64@0x140001000>
>>> current_function.clobbered_regs
RegisterSet(regs=['x0', 'x1', 'x2', 'x3', 'x4', 'x5', 'x6', 'x7', 'x8', 'x9', 'x10', 'x11', 'x12', 'x13', 'x14', 'x15', 'x16', 'x17', 'x18', 'x30', 'q0', 'q1', 'q2', 'q3', 'q4', 'q5', 'q6', 'q7', 'q16', 'q17', 'q18', 'q19', 'q20', 'q21', 'q22', 'q23', 'q24', 'q25', 'q26', 'q27', 'q28', 'q29', 'q30', 'q31'], confidence=192)140001000 sub_140001000:
140001000 ff4300d1 sub sp, sp, #0x10
140001004 f1000090 adrp x17, 0x14001d000
140001008 310a40f9 ldr x17, [x17, #0x10] {data_14001d010}
14000100c f16331cb sub x17, sp, x17 {var_10}
140001010 f10700f9 str x17, [sp, #0x8 {var_8}]
140001014 c0035fd6 ret 
CouleeApps
commented
2 years ago Looking further at this, it seems to make sense that the registers are clobbered. Since PPC uses r1 as the stack pointer register, assigning it a value based on r11 and then returning leaves the stack in an inconsistent state. In cases of an inconsistent stack on return, Binja reports all caller saved registers and return registers as clobbered, falling back to the calling convention's default behavior and not calculating a more precise clobbered register set.
With that said, I'm not sure of what the fix would be to this, or if this is even a bug. In many ISAs, if the stack is clobbered then returning can jump to an arbitrary address. This doesn't immediately seem to be the case with PPC, which has a dedicated link register, but if the caller saves it to the stack (as seems to be the convention), after this function returns it will restore the link register from an arbitrary address. So you would need interprocedural analysis to know what r11 is before entering this function, or you just give up analysis and make a conservative default (which is what binja does).
Re: Peter's analysis:
Re: yrp's comment: The function mentioned also modifies the stack pointer, thus hitting the same failure case described above.
plafosse
commented
1 year ago Given what @CouleeApps has reported I don't believe this is an analysis bug. Rather its a failure of the UI to show that we've hit this edge case and we're being conservative by marking everything as clobbered. The core might also need to be involved to set a flag when this condition occurs.
yrp604
commented
1 year ago I don't quite agree @CouleeApps's analysis. They're right that it would require interprocedural analysis to resolve, but not right that that it will restore the link register from an arbitrary address. With a bit of renaming, you can see what these routines are doing:
140001e28 int64_t sub_140001e28()
140001e28 fd7bbfa9 stp x29, x30, [sp, #-0x10]! {var_10} {__saved_x30}
140001e2c fd030091 mov x29, sp
140001e30 74fcff97 bl __security_push_cookie ; <-- stack is adjusted here, this is sub_140001000 above
140001e34 ff4300d1 sub sp, sp, #0x10
140001e38 e00300f9 str x0, [sp {var_20}]
140001e3c e9230091 add x9, sp, #0x8 {var_18}
140001e40 080080d2 mov x8, #0
140001e44 282100a9 stp x8, x8, [x9] {var_18} {0x0} {0x0}
140001e48 e10340f9 ldr x1, [sp {var_20}]
140001e4c e0230091 add x0, sp, #0x8 {var_18}
140001e50 a20b0094 bl sub_140004cd8
140001e54 e0230091 add x0, sp, #0x8 {var_18}
140001e58 bafdff97 bl sub_140001540
140001e5c e003002a mov w0, w0
140001e60 007c4093 sxtw x0, w0
140001e64 ff430091 add sp, sp, #0x10
140001e68 6efcff97 bl __security_pop_cookie ; <-- stack is readjusted here to re-align it before poping saved lr
140001e6c fd7bc1a8 ldp x29, x30, [sp], #0x10 {__saved_x30} {0x0}
140001e70 c0035fd6 ret Further, I don't think we can really consider this an edge case: this is the function that installs and checks stack cookies, meaning any non-trivial function will have this. Since this is the first chunk of code that gets called in a function, it is analysis impacting. For example, here is a random function from clobber.exe that has a stack cookie as its analyzed today:
140012160 int64_t sub_140012160()
14001217c int64_t x0
14001217c int32_t x1
14001217c double x2
14001217c int32_t x3
14001217c int32_t x4
14001217c int32_t x6
14001217c int64_t v0
14001217c int64_t v1
14001217c x0, x1, x2, x3, x4, x6, v0, v1 = __security_push_cookie() // clobber * happens here
140012188 int64_t var_f0 = v0
1400121b0 uint64_t x0_1 = _ctrlfp(0x9f00, 0x3c09f00)
1400121b8 uint64_t var_100 = x0_1
1400121c4 int64_t var_f8 = int.q(x2)
1400121cc if (_exception_enabled(x4, x0_1.d) != 0)
14001220c uint32_t x8_3 = zx.d(__acrt_has_user_matherr())
140012214 uint64_t x0_8
140012214 if (x8_3 == 0 || (x8_3 != 0 && x3 == 0))
140012258 _set_errno_from_matherr(zx.q(x3))
140012264 x0_8 = _ctrlfp(var_100.d, 0x3c09f00)
140012268 int128_t v0_2
140012268 v0_2.q = var_f8
140012214 if (x8_3 != 0 && x3 != 0)
140012218 int64_t v17_1 = var_f8
140012220 int64_t var_d8_1 = var_f0
140012220 int64_t var_d0_1 = v1
140012224 uint32_t x0_6 = var_100.d
140012228 int32_t var_e8 = x3
14001222c int64_t var_e0_1 = x0
140012234 _ctrlfp(x0_6, 0x3c09f00)
14001223c int128_t v0_1
14001223c x0_8, v0_1 = sub_140004f78(&var_e8)
140012240 if (x0_8.d == 0)
140012248 x0_8, v0_1 = _set_errno_from_matherr(zx.q(x3))
14001224c v0_1.q = v17_1
14001228c return __security_pop_cookie(x0_8)
1400121d4 if (x6 == 2)
1400121e0 int64_t var_90_1 = v1
1400121e8 int32_t var_80
1400121e8 int32_t var_80_1 = (var_80 & 0xffffffe0) | 3
140012204 void var_c0
140012204 sub_1400122a8(&var_c0, &var_100, x4, x1, &var_f0, &var_f8)
140012204 noreturnAnd here is that same function with the clobbers corrected via edit function properties:
140012160 uint64_t sub_140012160(int64_t arg1, int32_t arg2, double arg3, int32_t arg4, int32_t arg5, int64_t arg6, int32_t arg7, int64_t arg8)
14001217c __security_push_cookie()
140012188 int64_t var_f0 = arg8
1400121b0 uint64_t x0 = _ctrlfp(0x9f00, 0x3c09f00)
1400121b8 uint64_t var_100 = x0
1400121c4 int64_t var_f8 = int.q(arg3)
1400121cc int64_t v1
1400121cc if (_exception_enabled(arg5, x0.d) != 0)
14001220c uint32_t x8_3 = zx.d(__acrt_has_user_matherr())
140012214 uint64_t x0_7
140012214 if (x8_3 == 0 || (x8_3 != 0 && arg4 == 0))
140012258 _set_errno_from_matherr(zx.q(arg4))
140012264 x0_7 = _ctrlfp(var_100.d, 0x3c09f00)
140012268 int128_t v0_1
140012268 v0_1.q = var_f8
140012214 if (x8_3 != 0 && arg4 != 0)
140012218 int64_t v17 = var_f8
140012220 int64_t var_d8_1 = var_f0
140012220 int64_t var_d0_1 = v1
140012224 uint32_t x0_5 = var_100.d
140012228 int32_t var_e8 = arg4
14001222c int64_t var_e0_1 = arg1
140012234 _ctrlfp(x0_5, 0x3c09f00)
14001223c int128_t v0
14001223c x0_7, v0 = sub_140004f78(&var_e8)
140012240 if (x0_7.d == 0)
140012248 x0_7, v0 = _set_errno_from_matherr(zx.q(arg4))
14001224c v0.q = v17
140012270 __security_pop_cookie()
14001228c return x0_7
1400121d4 if (arg7 == 2)
1400121e0 int64_t var_90_1 = v1
1400121e8 int32_t var_80
1400121e8 int32_t var_80_1 = (var_80 & 0xffffffe0) | 3
140012204 void var_c0
140012204 sub_1400122a8(&var_c0, &var_100, arg5, arg2, &var_f0, &var_f8)
140012204 noreturnThings to specifically note:
plafosse
commented
1 year ago Yeah looking at this again I think you’re right. We need some better handling of clobbered registers when we run into this case.
Vague title, but I don't really know what the root issue is.
Repro: http://cyberjes.us/bash-ppc.elf @ 0x100c0a00
current_function.clobbered_regslists just about every register. The one that's particularly bothering me isr2, which is the global pointer register defined in the calling convention. This is breaking a bunch of references to constant data (r2 + offset).For context, 100c0a00 is part of the fairly standard "restore GPRs" sled compilers generate for PPC. Rather than doing your typical
pushfor each callee-saved register used in a function, PPC compilers tend to insert a sled of stores and a sled of loads, then functions will just branch to the right offset in the sled, e.g.save_r20_to_r31(); actual function code; restore_r20_to_r31();