search

bitdefender / bddisasm

bddisasm is a fast, lightweight, x86/x64 instruction decoder. The project also features a fast, basic, x86/x64 instruction emulator, designed specifically to detect shellcode-like behavior.
Apache License 2.0
888 stars 115 forks source link

Undefined behavior (misaligned load) #91

Closed turol closed 5 months ago

turol commented 5 months ago

Build bddisasm and disasmtool with ubsan and run ./disasmtool.bin -f ../bddisasm_test/x86/basic/address_16.test -b16

UBSan says 

bdx86_decoder.c:101:26: runtime error: load of misaligned address 0x7ffffd9bd6f5 for type 'ND_UINT32', which requires 4 byte alignment
0x7ffffd9bd6f5: note: pointer points here
 67 8b 84 79 00 10 00  00 00 00 00 00 00 00 00  8b 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00

Code is this: https://github.com/bitdefender/bddisasm/blob/63ca9e4328f706f070231951f4d7fb91f5569a6a/bddisasm/bdx86_decoder.c#L101

which ends up here: https://github.com/bitdefender/bddisasm/blob/63ca9e4328f706f070231951f4d7fb91f5569a6a/inc/bdx86_core.h#L383

It casts a pointer to a larger type without care about alignment. The C standard forbids this and there are cases where it will lead to miscompilation. It also makes it harder to check my code when there are errors in bddisasm.

19KaliNoob87 commented 5 months ago

I'm sorry I don't follow it has all just been too much. Ion what to do

On Tue, May 28, 2024, 9:45 AM Turo Lamminen @.***> wrote:

Build bddisasm and disasmtool with ubsan and run ./disasmtool.bin -f ../bddisasm_test/x86/basic/address_16.test -b16

UBSan says

bdx86_decoder.c:101:26: runtime error: load of misaligned address 0x7ffffd9bd6f5 for type 'ND_UINT32', which requires 4 byte alignment 0x7ffffd9bd6f5: note: pointer points here 67 8b 84 79 00 10 00 00 00 00 00 00 00 00 00 8b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Code is this:

https://github.com/bitdefender/bddisasm/blob/63ca9e4328f706f070231951f4d7fb91f5569a6a/bddisasm/bdx86_decoder.c#L101

which ends up here:

https://github.com/bitdefender/bddisasm/blob/63ca9e4328f706f070231951f4d7fb91f5569a6a/inc/bdx86_core.h#L383

It casts a pointer to a larger type without care about alignment. The C standard forbids this and there are cases where it will lead to miscompilation. It also makes it harder to check my code when there are errors in bddisasm.

— Reply to this email directly, view it on GitHub https://github.com/bitdefender/bddisasm/issues/91, or unsubscribe https://github.com/notifications/unsubscribe-auth/BG7QNUUCTUSQRSMBCDH4P2TZESJ7JAVCNFSM6AAAAABINDAPI2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGMZDCMRXGEYDGNQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

turol commented 5 months ago

The correct way to do this is to use memcpy

uint32_t target;
memcpy(&target, b, 4);
vlutas commented 5 months ago

Hello,

Indeed, that might lead to unaligned accesses. While not necessarily a problem (modern OSes and architectures allow unaligned accesses), this certainly needs fixing, as it is undefined behavior.

Latest commit contains the fix. Thanks for reporting this!

19KaliNoob87 commented 5 months ago

Wow thanks. I have to fix the non bootable device

On Tue, May 28, 2024, 11:22 AM vlutas @.***> wrote:

Hello,

Indeed, that might lead to unaligned accesses. While not necessarily a problem (modern OSes and architectures allow unaligned accesses), this certainly needs fixing, as it is undefined behavior.

Latest commit contains the fix. Thanks for reporting this!

— Reply to this email directly, view it on GitHub https://github.com/bitdefender/bddisasm/issues/91#issuecomment-2135660715, or unsubscribe https://github.com/notifications/unsubscribe-auth/BG7QNUVIDBYJNHR3CBUWLA3ZESVNZAVCNFSM6AAAAABINDAPI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVGY3DANZRGU . You are receiving this because you commented.Message ID: @.***>