Assert call being removed during compilation, causing NULL pointer dereference in ble_ll_task

Verification

[X] I searched for similar bug reports (including closed issues) and found none was relevant.

What happened?

For some reason, one of the asserts in ble_ll_task function is getting optimized out during compilation which causes NULL pointer dereference in the subsequent function call.

What should happen instead?

Assert failure should occur if the event is null.

Reproduction steps

Relevant part of code from ble_ll.c

This particular assert before calling the function pointer from the event object recieved -

 while (1) {
        ev = ble_npl_eventq_get(&g_ble_ll_data.ll_evq, BLE_NPL_TIME_FOREVER);
        assert(ev);
        ble_npl_event_run(ev);
    }

Compile the application with -

mkdir build && cd build
cmake -DARM_NONE_EABI_TOOLCHAIN_PATH=$PWD/../gcc-arm-none-eabi-10.3-2021.10 -DNRF5_SDK_PATH=../nRF5_SDK_15.3.0_59ac345 ..
make pinetime-app -j20

After compilation , load the pinetime-app.out ARM executable in ghidra

Here is the decompilation of the function of interest -

void ble_ll_task(void)

{
  int iVar1;

  ble_phy_init();
  ble_phy_txpwr_set(0);
  ble_hci_trans_cfg_ll(0x25795,0,0x257b5);
  ble_ll_hci_send_noop();
  do {
    iVar1 = npl_freertos_eventq_get(&DAT_2000dd08,0xffffffff);
    (**(code **)(iVar1 + 4))();
  } while( true );
}

Atleast on the disassembly level, the assert check is missing. While testing, I have trace from gdb and a not so useful segmentation fault from ASAN trace -

$eax   : 0x0
$ebx   : 0x2
$ecx   : 0x9b4a
$edx   : 0xf7fbd298  →  0x00000000
$esp   : 0xf7faf230  →  0x0882cab4  →  <__start___sancov_guards+158588> dec edx
$ebp   : 0xf7faf248  →  0xf7faf278  →  0xf7faf298  →  0xf7faf2b8  →  0xf7faf328  →  0x00000000
$esi   : 0x0
$edi   : 0x0
$eip   : 0x08233368  →  <ble_npl_event_run+104> mov eax, DWORD PTR [eax+0x4]
$eflags: [zero carry parity adjust sign trap INTERRUPT direction overflow RESUME virtualx86 identification]
$cs: 0x23 $ss: 0x2b $ds: 0x2b $es: 0x2b $fs: 0x00 $gs: 0x63
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stack ────
0xf7faf230│+0x0000: 0x0882cab4  →  <__start___sancov_guards+158588> dec edx      ← $esp
0xf7faf234│+0x0004: 0xf7fbd258  →  0x0000005e ("^"?)
0xf7faf238│+0x0008: 0xf7fbd298  →  0x00000000
0xf7faf23c│+0x000c: 0x00000000
0xf7faf240│+0x0010: 0x00000000
0xf7faf244│+0x0014: 0x00000000
0xf7faf248│+0x0018: 0xf7faf278  →  0xf7faf298  →  0xf7faf2b8  →  0xf7faf328  →  0x00000000       ← $ebp
0xf7faf24c│+0x001c: 0x082331e4  →  <ble_ll_task+244> jmp 0x8233187 <ble_ll_task+151 at /home/ritvik/Desktop/Files/Fuzzing_RTOS_Applications/Ported_Applications/dataset/FreeRTOS_Apps/FreeRTOS_Apps_Ported/InfiniTime_ported/src/libs/mynewt-nimble/nimble/controller/src/ble_ll.c:1207>
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── code:x86:32 ────
    0x823335d <ble_npl_event_run+93> mov    DWORD PTR [esp], eax
    0x8233360 <ble_npl_event_run+96> call   0x877c960 <__sanitizer_cov_trace_pc_guard>
    0x8233365 <ble_npl_event_run+101> mov    eax, DWORD PTR [ebp-0x8]
 →  0x8233368 <ble_npl_event_run+104> mov    eax, DWORD PTR [eax+0x4]
    0x823336b <ble_npl_event_run+107> mov    ecx, DWORD PTR [ebp-0x8]
    0x823336e <ble_npl_event_run+110> mov    DWORD PTR [esp], ecx
    0x8233371 <ble_npl_event_run+113> call   eax
    0x8233373 <ble_npl_event_run+115> add    esp, 0x14
    0x8233376 <ble_npl_event_run+118> pop    esi
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────── source:/home/ritvik/De[...].h+117 ────
    112  }
    113
    114  static inline void
    115  ble_npl_event_run(struct ble_npl_event *ev)
    116  {
             // ev=0xf7faf240  →  0x00000000
 →  117      ev->fn(ev);
    118  }
    119
    120  static inline bool
    121  ble_npl_eventq_is_empty(struct ble_npl_eventq *evq)
    122  {

and ASAN trace -

Continuing.
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==1500903==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x00000004 (pc 0x08233368 bp 0xf7faf248 sp 0xf7faf230 T1500988)
==1500903==The signal is caused by a READ memory access.
==1500903==Hint: address points to the zero page.
    #0 0x8233368  (/home/ritvik/Desktop/Files/test/infinitime_ins+0x8233368)
    #1 0x82331e3  (/home/ritvik/Desktop/Files/test/infinitime_ins+0x82331e3)
    #2 0x867d110  (/home/ritvik/Desktop/Files/test/infinitime_ins+0x867d110)
    #3 0x812b86e  (/home/ritvik/Desktop/Files/test/infinitime_ins+0x812b86e)
    #4 0xf7886c00  (/lib/i386-linux-gnu/libc.so.6+0x86c00)
    #5 0xf792372b  (/lib/i386-linux-gnu/libc.so.6+0x12372b)

UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV (/home/ritvik/Desktop/Files/test/infinitime_ins+0x8233368)
==1500903==ABORTING

I'm sure this might be something trivial I'm missing, but kindly verify this on your end and let me know.

More details?

No response

Version

This is the commit version - 004b2bf3a019fa63e18b0b2d5bd912ebf82c8c76

Companion app

No response

Generally at least in C family languages asserts are semantically meant to be removed for release/optimised builds - they should only be used for debugging by developers and therefore don't need to be included in user facing builds.

However there's certainly some abuse of asserts in InfiniTime where error checking relies on asserts that are removed for release builds e.g verifying the size of DFU packets is done with an assert.

TLDR: Not a bug that the assert is removed, but definitely a bug if it's being hit in a debug build

I'm entirely not sure if an assert failure has happened in the debug environment you have for testing, but during my testing, I see the assert failure happening even in the debug build.

infinitime_ins: ~/InfiniTime_ported/src/libs/mynewt-nimble/porting/npl/freertos/src/npl_os_freertos.c:42: struct ble_npl_event *npl_freertos_eventq_get(struct ble_npl_eventq *, ble_npl_time_t): Assertion `tmo == 0' failed.

Thread 4 "ll" received signal SIGABRT, Aborted.
0xf7fc4579 in __kernel_vsyscall ()
[ Legend: Modified register | Code | Heap | Stack | String ]
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── registers ────
$eax   : 0x0
$ebx   : 0x1bf446
$ecx   : 0x1bf4d0
$edx   : 0x6
$esp   : 0xf7fa9f60  →  0xf7a2ad00  →  0xfbad2887
$ebp   : 0xf7a2ad00  →  0xfbad2887
$esi   : 0x1bf4d0
$edi   : 0xf7faaac0  →  0xf7faaac0  →  [loop detected]
$eip   : 0xf7fc4579  →  <__kernel_vsyscall+9> pop ebp
$eflags: [ZERO carry PARITY adjust sign trap INTERRUPT direction overflow resume virtualx86 identification]
$cs: 0x23 $ss: 0x2b $ds: 0x2b $es: 0x2b $fs: 0x00 $gs: 0x63
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stack ────
0xf7fa9f60│+0x0000: 0xf7a2ad00  →  0xfbad2887    ← $esp
0xf7fa9f64│+0x0004: 0x00000006
0xf7fa9f68│+0x0008: 0x001bf4d0
0xf7fa9f6c│+0x000c: 0xf7888aa7  →   mov ebp, eax
0xf7fa9f70│+0x0010: 0x00000000
0xf7fa9f74│+0x0014: 0x00000013
0xf7fa9f78│+0x0018: 0xf7a2a000  →  0x00229dac
0xf7fa9f7c│+0x001c: 0x00000006
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── code:x86:32 ────
   0xf7fc4573 <__kernel_vsyscall+3> mov    ebp, esp
   0xf7fc4575 <__kernel_vsyscall+5> sysenter
   0xf7fc4577 <__kernel_vsyscall+7> int    0x80
 → 0xf7fc4579 <__kernel_vsyscall+9> pop    ebp
   0xf7fc457a <__kernel_vsyscall+10> pop    edx
   0xf7fc457b <__kernel_vsyscall+11> pop    ecx
   0xf7fc457c <__kernel_vsyscall+12> ret
   0xf7fc457d <__kernel_vsyscall+13> int3
   0xf7fc457e                  nop

On some naive analysis on my end, I feel it would be better if the assert call was replaced with a simple check for NULL event and a return. Since the application is trying to call a function pointer after the assert, it makes more sense to have a check and return instead of assert. But like I said, it can be a purely hypothetical case where the event is returned as a NULL pointer. However, hitting a null dereference in release build of the application breaks it's functionality. I trust that the developers would have made an informed decision before placing an assert here.

Hmm, I don't really know enough about the structure of nimble to say more now. You probably already know, but in case not nimble is a 3rd party library InfiniTime relies on, so unless we're using it wrong the bug is external to this project. Either way, I think the assertion wouldn't be hit here because ev is not a null pointer. It is ev->fn that seems to be problematic.

The assert you have firing above is in a different location. This assert checks that any queue read from an ISR specifies a zero timeout (non blocking), as you cannot block inside an ISR. But something has in fact called the read function inside an ISR with a timeout set (i.e expecting to wait some time for an item of the queue) and may be about to unexpectedly get no item. AFAIK the LL task never runs from an interrupt context, so it shouldn't ever cause that assert to fire

Thanks for reporting this issue @PwnVerse !

As @mark9064 said, asserts are not compiled into the final firmware in Release build, so this is expected that you cannot find it when disassembling the binary. If you build in Debug mode, the assert should be built into the firmware.

The part of the code you mentioned is part of the NimBLE BLE stack so if you find issues and bugs in this project, they would probably be happy to receive a report as well. Please note that we are probably using an older version of the library since we haven't updated our copy for quite some time now.

I've never used Ghidra and ASAN (but I definitely would like to learn them at some point in time!). Could you give more detail about how you detected this issue? Do you run InfiniTime with ASAN on the PineTime ? Or do you run InfiniTime code on your computer? Also, does this NULL pointer dereference happen at runtime, or is it a theoretical behavior?

I ran the Infinitime pinetime app as a native linux application. The NULL pointer dereference happens at runtime. But as @mark9064 mentioned, I agree that this assert failure would not occur since the ll task would never be run from an ISR. I had manually changed the value of the MMIO register so that it is tricked into behaving as if it is in an ISR and that raises an assert on the timeout value (in the debug build) later causing a NULL deref (in the Release build). Anyways, you can close this issue as it is not actually a bug. As a final note, it would be nice to see some of the asserts being replaced with simpler if checks so that the application actually does not crash in release builds (for actual bugs).

Thanks for the additional information! I'll close this issue since it's not likely to happen when it's running on the actual hardware.

As I previously said, this specific assert is part of the code of NimBLE, so you might want to see what is their position about asserts in the code. I don't think there're much (if any) asserts in the InfiniTime codebase. I typically use them as a "should never happen" condition that I can still catch when debugging/unit testing, but I don't like them either.

InfiniTimeOrg / InfiniTime