Please restore our registers when you’re done with them

https://randomascii.wordpress.com/2022/11/21/please-restore-our-registers-when-youre-done-with-them/

by jmillikin • 4 years ago

244 129 4 years ago

randomascii.wordpress.com

bombcar4• 4 years ago

It’s interesting that there is a zero stored in a register and used for hours - is that significantly faster than just using some actual zero each time? Perhaps CPUs need a “always zero” register or some similar menomic to help harden.

gumby4• 4 years ago

Many architectures do have a 0 register because the value is useful. Others have a zero instruction (or both).

What would an "actual zero" be -- a literal?

bombcar4• 4 years ago

Yeah, something like "mov ax, 0h" but I suppose that is way more memory intensive as you have to load a 0 into memory somewhere and then copy it into the register.

It strikes me as somehow the compiler is making assumptions that aren't being enforced by the ... OS? Language? not sure what, but it's assuming functions restore registers used but that isn't enforced by anything. From my (long ago) time there was PUSHA and POPA but I assume those take quite a bit of "oomph" and are avoided if possible.

pitaj• 4 years ago

I don't know anything about x86, but on Arm you can use an immediate 0 that is stored in the instruction data.

Veliladon2• 4 years ago

Intel has never really needed to have a zero register because xor register, register as a zeroing idiom is so fast and so recognized that Intel have optimized the hell out of it. In Sandy Bridge and onward it doesn't even go through an execution port, even for the vector registers.

The problem is really whether to indulge bad programmers who don't respect the ABI at the cost of a minimal sliver of performance (even though it's not taking up an execution port the extra instruction still takes up cache space, bandwidth, and decode). Yeah they should probably zero the register before they zero the pointer but they shouldn't have to if other people respected the ABI.

kazinator• 4 years ago

I think it's not just the xor trick, but that Intel has lots of addressing modes, including ones with immediate operands that you can use in many situations.

In RISC-like machines, most of the operations are register-register, and you have load/store instructions for referencing memory.

To use an immediate operand (literal constant in the code itself), you may have to load it into a register, like

   move r7, #42
   add r1, r1, r7  ;; ok, now we have 42 in r7, we can increment r1 by 42.

Whereas in a CISC you would have

   add r1, #42  ;; two operand form

or maybe

   add r1, r1, #42 ;; three operand form

When you need a zero, you just use the immediate operand zero, and thus you don't need to to pick some register to clear.

In summary, zero registers in RISC-like instruction set architectures effectively provide a literal zero that can be used wherever a register is required, which helps because only register operands can be used in many instructions.

CodesInChaos• 4 years ago

RISC-V has such a register. It returns zero when read, and ignored writes.

ddingus3• 4 years ago

Where it's not optimized away, getting "an actual zero" requires a memory operation of some kind. Register ops are faster in that they are right there, no fetch needed.

AnimalMuppet4• 4 years ago

Depends on the instruction architecture. 68000 had some ways of burying a small literal operand in the instruction. If I recall correctly, MOVEQ.L would let you move zero to a register without touching memory (other than the instruction fetch), and it wasn't a long instruction.

mkj• 4 years ago

It seems that the chrome developers should be able to perform the same binary analysis on the suspect Mcafee software. I guess it's a bit harder without source code to reference side-by-side though.

brucedawson16• 4 years ago

That would be possible, but we'd have to install the software, then guess which binary was the culprit, and then have some way of finding the function boundaries. My crude analysis technique required on having symbols for chrome.dll to indicate where functions started, so I'd have to have switched tools to something else that could find those.

saagarjha• 4 years ago

Most disassemblers are fairly decent at guessing function boundaries if the code isn't too bad.

webstrand• 4 years ago

I wouldn't be surprised if that was a license violation

pjc503• 4 years ago

One of the linked pull requests: https://github.com/cisco/openh264/commit/db956674bbdfbaab5ac...

Macros in a macro assembler .. not very nice.

userbinator• 4 years ago

He even mentions that it's Windows-specific, both in the commit message and in the article... and then seemingly fails to make it Windows-only? That's "not very nice" either.

electroly• 4 years ago

Those PUSH_XMM/POP_XMM macros appear to be Windows-only; I think they expand to nothing on other platforms because they contain their own guard for Windows internally. If that's the case, the call sites don't need to guard for it. I'm guessing that obeying this calling convention is the purpose of those macros.

https://github.com/cisco/openh264/blob/db956674bbdfbaab5acdd...

323• 4 years ago

Sounds like there should be an option when you write assembly code to tell the compiler "please save/restore any register that I'm modifying in this asm code according to the target you are compiling for"

pjc503• 4 years ago

That kind of feature is very .. un-assembler. You certainly could, but the assembler doesn't keep track of any of the relevant information, so it would be a larger feature than you expect. It doesn't know the calling convention. It doesn't have a map of registers dirtied by which instructions. It doesn't do reachability analysis, so it doesn't even necessarily know what's "in the function".

Joker_vD• 4 years ago

Doesn't GCC inline assembly actually track that info, with the "clobber" section?

dfox• 4 years ago

THat is essentially how inline assembly in GCC works. You have to declare which registers you are changing and in which registers you expect input and which contain results from your inline assembly block.

This for obvious reasons does not work if you have separate compilation unit written in assembly, then you have to follow the ABI.

flanfly• 4 years ago

I not sure whether that satire. In case its not, most languages that allow inline assembly (like C) have an optional "clobber list" argument that tells the dataflow analysis of the compiler that your assembly snippet overwrites certain registers [1]. Inline assembly doesn't have target specific clobber lists because it's assumed that the code only works on one target and the programmer has to take care of making it work.

1: https://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO....

tom_• 4 years ago

The System V x64 ABI is different from the Windows x64 ABI.

matthews2• 4 years ago

Maybe we need some kind of higher level language that gets boiled down into assembly.

pencilguin• 4 years ago

And, a compiler that knows custom SIMD optimizations for every algorithm anyone might ever need, and can recognize when you have coded one of them so it can substitute its SIMD version.

astrange2• 4 years ago

C does have official intrinsics for SIMD on Intel platforms, but the people who are good at writing video codecs don't like to use them because Wintel culture has such bad taste at naming functions (thanks to Hungarian notation) that using them is near-unreadable and it's easier to write everything in asm.

wyldfire• 4 years ago

When you use inline assembly, in fact the compiler does preserve the semantics of the surrounding program considering the target's ABI -- if you tell it correctly what impacts the inline asm has. Lots of rules must be followed in order to get this behavior just right. One of the most subtle ones is "early clobbers" [1].

In many cases, you can get all the benefits of inline assembly from compiler intrinsics while letting the compiler handle all the details of register allocation and scheduling.

Note that in OP's case IIUC this was assembly code and not inline assembly. If you write functions in assembly you are solely responsible for calling conventions and ABI conformance.

[1] https://stackoverflow.com/a/15819941/489590

mrkeen2• 4 years ago

You could totally design a better convention in your own high-level language which compiled down to assembly, as long as you stayed inside it. But once you need to interface with external code you need to use a shared convention.

Your scheme sounds like it would make your code well-behaved as the callee (with perhaps some performance penalty?). But as a caller, you couldn't trust the external code not to clobber registers.

brucedawson16• 4 years ago

In fact the fix to the WebRTC bug was to adjust the clobber list, thus telling the compiler to save the registers.

Why the compiler didn't notice that registers were being used that weren't on the clobber list is unclear to me. Your suggestion seems totally reasonable.

https://chromium-review.googlesource.com/c/libyuv/libyuv/+/3...

rocqua• 4 years ago

To what degree is this possible to check statically?

It feels like at least simple breaks of the ABI rules like this can be detected somewhat statically. The author already started with a very simple and incomplete version.

In general, I wonder, are there any (many?) static analyzers for assembled binaries.

fweimer• 4 years ago

Compilers do inter-procedural register allocation and use custom calling conventions for local calls (where “local” can be quite large with LTO), while preserving ABI externally. This means that clobbering a callee-saved register without saving/restoring it in the same function is not necessarily a bug.

Curiously, I found a register clobber bug in the NaCl cryptography library today. Apparently, they used a custom assembler-preprocessor (qhasm) that avoids certain classes of bugs and aids with porting, but while the tool seems to actually model the register in some way, it does not treat it as callee-saved.

brucedawson16• 4 years ago

You could write an arbitrarily complex analyzer to try to find violations but I suspect that the halting problem means that you can never be sure you've found all errors. I think that either crude heuristics (found two bugs!) or UBSan style instrumentation (finds all bugs in code executed under test) is the best set of solutions.

secondcoming• 4 years ago

Ugh, I had a similar issue when I forgot to clobber ecx when using inline assembly for rdtscp. Very annoying.

codeflo• 4 years ago

Why does a „3rd party encryption software” appear in the call stack of a user mode process in the first place? Is this another case where a “security” software injects broken DLL files into all processes in your system?

cpeterso• 4 years ago

Yes. That's pretty common, unfortunately.

JoshTriplett4• 4 years ago

I'd love to see Chrome and Firefox and IE working together to detect this kind of thing and put up a malware warning explaining how to uninstall it, and see how quickly it can be eliminated.

Seriously, there is zero valid reason to ever inject code into another program, other than as a debugging tool on a system being debugged.

Cloudef3• 4 years ago

The irony of security and anti-cheat software acting like a invasive malware

dblohm73• 4 years ago

I used to take point for Mozilla's efforts dealing with third-party interference in our binaries. Browsers are ripe targets for this kind of shit. The stories we could tell...

harry8• 4 years ago

And from our perspective you should! Would be interesting.

dblohm73• 4 years ago

I did write a little bit! Here's an oldie but a goodie:

https://dblohm7.ca/blog/2016/01/11/bugs-from-hell-injected-t...

srcreigh• 4 years ago

Please write some stuff about this

dblohm73• 4 years ago

I did write a little bit! Here's an oldie but a goodie:

https://dblohm7.ca/blog/2016/01/11/bugs-from-hell-injected-t...

otikik2• 4 years ago

Naïve question about ABIs: shouldn’t the caller be responsible for that? If I want a function to restore certain registers, wouldn’t it be simpler if I was the one that save them on my memory, call the function, and then override the registers with whatever values the function set? Otherwise it seems we’re just… asking for trouble, so to speak.

dannymi• 4 years ago

It would just be slow for the caller to have to push and pop (say 30) registers in general that the specific callee (and transitive callees) may not even use.

Most ABI specify some registers caller-saved, some registers callee-saved (retained unchanged from the perspective of the caller) and some registers scratch (not-saved).

In the end it depends on the architecture and on typical workload which are the fastest--and measurements can be made and it can be found out which combination is the fastest on average.

dzaima2• 4 years ago

You usually wouldn't need to push&pop all registers, just ones that you want to preserve across the call. Regardless, yeah, non-volatile aka callee-saved registers are extremely important for good performance of code that calls functions (esp. loops - without callee-saved registers, you'd have to store the loop counter & length on the stack!)

chrisseaton• 4 years ago

If I want a function to restore certain registers, wouldn’t it be simpler if I was the one that save them on my memory, call the function, and then override the registers with whatever values the function set?

That's a waste if the callee doesn't use them.

otikik2• 4 years ago

Indeed. But isn’t it also a waste when someone doesn’t follow through and then we have situations like the one described on the article?

asveikau• 4 years ago

ABIs are a convention that everybody follows regarding how such preservation should or should not occur, for which registers.

It's common to say "caller cleans up registers x, y, z, callee cleans up a, b, c".

So yes, the caller could do it or not do it, the choices are all possible to do both ways, but that wouldn't be the agreed upon ABI, it'd be something different.

von_lohengramm• 4 years ago

I'm sure some C++ lawyer can correct me, but isn't branching off of `m_ptr` (e.g. `CHECK()`ing the value) after `std::move(m_ptr)` technically Unspecified Behavior since `std::move(m_ptr)` leaves `m_ptr` as an Unspecified Value? It would be up to the compiler to define the behavior if they so pleased, but the C++ spec would not require such behavior to be defined at all.

tux3• 4 years ago

Moves in C++ don't make the whole object invalid, it just leaves them in a valid but unspecified state, so that at least the destructor can still run.

This means calling operator bool on your unique_ptr ought to be fine, because the unique_ptr still has a valid state (you don't know what that state is, it's unspecified, but it's guaranteed to not be radioactive on mere contact. It has to be a valid unspecified state.)

account425• 4 years ago

Moves in C++ are a function call that can do whatever any other function call can. The type itself can have additional guarantees and the standard library types generally do. In the case of std::unique_ptr the guarantee is explicit that the moved-from unique_ptr is nulled.

twoodfin• 4 years ago

It’s an unspecified but required to be valid value for the moved type. The author mentions it’s a smart pointer type, which could easily be defined to act like this.

CamperBob2• 4 years ago

It appears that you need to be really smart in order to not blow your own foot off with this "smart pointer." I'll stick to the regular dumb kind, thanks.

s28l• 4 years ago

There is a big difference between "undefined" and "unspecified" behavior. In this case, the behavior of `unique_ptr(unique_ptr&&)` is in fact specified. [0]

However, the bigger issue with that code is that it can easily stop working with a simple refactor. Consider:

    void foo(std::unique_ptr<int> ptr) {}
    void bar(std::unique_ptr<int>&& ptr) {}

    int main()
    {
        std::unique_ptr<int> p1{new int{1}};
        std::unique_ptr<int> p2{new int{2}};

        foo(std::move(p1));
        assert(p1 == nullptr);

        bar(std::move(p2));
        assert(p2 != nullptr);
     }

Neither of the above asserts will fire, but from the calling site, they look exactly the same. In my opinion, the more explicit option would be to do something like `bar(std::exchange(p2, nullptr))`

[0]: overload (5) https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_p...

account425• 4 years ago

Right, but changes in some other code potentially silently breaking assumptions at the calling site is exactly why you would want an assert like this in the first place.

brucedawson16• 4 years ago

I was wondering if anybody was going to point that out. It did occur to me that the CHECK was not technically valid due to that exact concern, but given that we control the compiler and the C++ library implementation and given that it's just debugging code (albeit debugging code that we ship to users) I'm fine with it.

In other words, I guess you shouldn't oughta do that generally, but I was fine with it being used there, and it did its job.

brucedawson16• 4 years ago

It appears that the question of what is valid on a moved-from object is tricky. Here is one discussion: https://stackoverflow.com/questions/7027523/what-can-i-do-wi... FWIW, here is the move operator for the type of the object in question: https://source.chromium.org/chromium/chromium/src/+/main:bas...

bumholio• 4 years ago

If one could reliably detect this DLL injection in the process address space, then the correct "fix" is to crash immediately. Authors of such tools should seek another way of accomplishing their goal, preferably one that does not export their own bugs to innocent bystanders.

dreamcompiler3• 4 years ago

In this particular example, why not just

  PXOR XMM7 XMM7

before using XMM7 to zero anything? That way the compiler doesn't have to assume that XMM7 is zero, it can know.

Yes it's an extra instruction but XORing a register with itself is such a common metaphor for zeroing that register that CPU designers try to make it fast.

Edit: Just noticed that Veliladon essentially made the same comment herein and explained the reason why it's not done this way.

puffoflogic• 4 years ago

It's not just that. If you can't trust the ABI then everything else is wrong too. Everything. Not just zeroing registers. It only just so happens that in this case XMM7 is used for zeroing, but it could be used to save a variable across a function call. Then there is no trick to get it set back to the right value.

The ABI is not optional, or best effort, or best practice, or any other BS that passes in the ordinary world. It is just as required as the correct operation of instructions (e.g., add should actually add things, mul should actually multiply them, and so on).

mrkeen2• 4 years ago

My reading of it was that the bug was caused by XMM7 not being restored to its previous value.

E.g. on Linux, functions should restore the values of ebx, esi, edi, ... once they're done with them. The article says (on Windows) that XMM7 needs to be restored too.

If you can't trust one register being preserved (per the ABI), then you really can't trust the values of any registers.

dreamcompiler3• 4 years ago

Fair point, which is why I used the phrase "this particular example." In fact the compiler's lazy assumption about XMM7 was the key to determining that the ABI was being violated. It would have taken longer to figure this out if the compiler had been doing things my way, because then a failure of the callee to preserve XMM7 wouldn't have mattered.

pifm_guy• 4 years ago

I think the real fix is to set all registers to a canary value at the start of int main(), and then when exiting check the canary value is still there.

If anything has messed with any registers without permission, you crash and collect as much data as possible about any injected dll's.

Then you correlate these to find the culprits, and for each you contact the authors of the DLL and figure out a way to block the injection of any unfixed versions that cause crashes.

TillE• 4 years ago

Code can be injected anywhere. You'd have to add checks to every single function.

andreareina• 4 years ago

Any good function upstream of the bad one that also uses the same register will mask the issue

ip26• 4 years ago

Each runtime function call would probably need its own unique value, perhaps stashed to memory.

brucedawson16• 4 years ago

McAfee/Trellix says that they have fixed the bug in the latest (7.4.0) version of their disk encryption product. So apparently my guesses about the root cause were correct.

Please restore our registers when you’re done with them

Related Stories