They say that the devil is in the details. Now, while the OS X kernel bug described here can hardly be classified as ‘devilish’ (it’s rather prosaic), it does illustrate how seemingly simple hand-coded assembly routines can contain flaws. Since Apple has decided not to fix this bug, let’s take a peek at it – because who doesn’t like spelunking around the kernel looking at buggy code?
While the majority of the kernel proper is written in C, some parts of OS X (XNU) are written in assembly. There are a variety of situations where it makes sense to use hand-written assembly (for example direct hardware access or optimizations). Looking at the source code for XNU reveals several memory copy routines that have been implemented in assembly for optimization reasons. _bcopy, implemented within locore.s is one such hand-coded assembly routine. Though quite simple, it contains a subtle bug.
The _bcopy function is implemented within locore.s , and is used to “copyin/out from user/kernel address space”. It’s basically analogous to memcpy.
_bcopy is invoked from within the copyio function in order to handle various copy operations. Looking at copyio.c , the source code file where copyio is implemented, _bcopy is declared in the following manner:
Then _bcopy is invoked in order to copy data from one buffer to another:
Examining the _bcopy code reveals its simplicity; it’s only 10 assembly instructions!
Let’s take a closer look at the disassembly. As is shown in the previous figure, first RDI and RSI are swapped, then the direction flag is cleared. Following this EDX is moved into ECX, which is then shifted right by three. This sets up rep MOVSQ which will copy some number (ECX) of ‘quadwords’ from RSI to RDI. Any remaining bytes are then copied via REP MOVSB. Following this, the code zeros out EAX and returns. These series of instructions should look quite familiar to reverse engineers who have seen disassemblies of inlined-memcpys.
So did you spot the bug? Though it was initially uncovered via single-stepping thru the kernel, looking at the source code reveals a ‘TODO’ that points towards the bug as well:
Recall that _bcopy’s declaration states the size variable is of type vm_size_t and that the copyio function invokes _bcopy with the ‘nbytes’ argument (which is also declared as a vm_size_t). On x86_64 systems, vm_size_t is 64 bits in size…not 32 bits. In other words, code that invokes _bcopy expects the ‘number of bytes to copy’ variable to be treated as a 64-bit value – not truncated down to 32 bits. Truncation is generally a bad thing and in C source code, will generate compiler warnings. However hand-coded assembly may not generate such warnings and thus, such issues may be overlooked.
In theory this bug could lead to issues where data is only partially copied or not fully overwritten/zero-out (info leak anyone?).
This bug was reported to Apple in December of 2013. Although Linus thinks all bugs are equal, apparently Apple does not. The buggy code is still found within the Yosemite kernel. To fix the bug, the 64-bit versions of the registers should be used. Specifically, line 3 of _bcopy should be replaced with movq %RDX, %RCX. By using the 64-bit versions of the registers no bytes are truncated. With this simple fix in place, the code will always execute as expected: the correct number of bytes will be copied.
Interestingly, Apple did get things right in bcopy (note the missing leading underscore in this name) implemented in the file bcopy.s :
Since bcopy is correctly implemented, Apple could avoid the bug altogether, by simply removing _bcopy and updating code to instead invoke bcopy (for example in copyio()).
Writing routines in assembly code can provide optimizations. However, it is error prone and difficult to maintain. In the OS X kernel one such routine, the humble _bcopy has a bug. While this bug isn’t likely a security issue, (and unlikely to come to fruition under normal circumstances due to buffer size limitations) anytime code within the kernel is buggy, bad things could possible happen.
Even though it’s a one-line fix, will this bug ever be eradicated? Who knows…
cvv stores cc cvv
