NULL deref in ble_gap_rx_rd_rem_ver_info_complete if the conn is already gone

[JakeMan314]

Hit a kernel panic on ESP32 (nimble_host) while connecting as a client. Decoded it back to ble_gap_rx_rd_rem_ver_info_complete writing through conn without checking if ble_hs_conn_find actually found anything.

Note: a portion of this fix was AI generated, but we did see these issues surface on real devices.

NimBLE-Arduino 2.3.7, ESP32 Xtensa, Arduino/PlatformIO.

Crash PC 0x400f845f:

0x400f845f  ble_gap_rx_rd_rem_ver_info_complete     ble_gap.c:2863
0x400fd554  ble_hs_hci_evt_rd_rem_ver_complete      ble_hs_hci_evt.c:767
0x400fd9a1  ble_hs_hci_evt_process                  ble_hs_hci_evt.c:1345
0x400fc521  ble_hs_event_rx_hci_ev                  ble_hs.c:552
0x40104785  ble_npl_event_run / nimble_port_run     nimble_port.c:297
0x400f31a7  NimBLEDevice::host_task                 NimBLEDevice.cpp:886

The code in question:

https://github.com/h2zero/NimBLE-Arduino/blob/2.3.7/src/nimble/nimble/host/src/ble_gap.c#L2851-L2875

conn = ble_hs_conn_find(le16toh(ev->conn_handle));

ble_hs_unlock();

conn->bhc_rd_rem_ver_params.version = ev->version;
conn->bhc_rd_rem_ver_params.manufacturer = ev->manufacturer;
conn->bhc_rd_rem_ver_params.subversion = ev->subversion;

if ((conn != NULL) &&  !(conn->bhc_flags & BLE_HS_CONN_F_MASTER)) {

So it looks up the conn, unlocks, then immediately does conn->... anyway. The NULL check is a few lines later, after the stores. If the handle is already gone (disconnect raced the rem-ver complete event), that's a panic.

ble_gap_rx_rd_rem_sup_feat_complete right above it does check conn != NULL first. On the master path that function then calls ble_gap_rd_rem_ver_tx(), which is how we end up in this one. Client connect, peripheral drops / connect timeout / deleteClient before rem-ver complete, HCI event still shows up, boom.

Also unlocking before touching conn is a UAF even when it isn't NULL.

Fix is just: check NULL (and copy the rem-ver fields) while still holding the lock.

void
ble_gap_rx_rd_rem_ver_info_complete(const struct ble_hci_ev_rd_rem_ver_info_cmp *ev)
{
#if NIMBLE_BLE_CONNECT
    struct ble_hs_conn *conn;
    int is_master;

    ble_hs_lock();
    conn = ble_hs_conn_find(le16toh(ev->conn_handle));
    if (conn == NULL) {
        ble_hs_unlock();
        return;
    }

    conn->bhc_rd_rem_ver_params.version = ev->version;
    conn->bhc_rd_rem_ver_params.manufacturer = ev->manufacturer;
    conn->bhc_rd_rem_ver_params.subversion = ev->subversion;
    is_master = conn->bhc_flags & BLE_HS_CONN_F_MASTER;
    ble_hs_unlock();

    if (!is_master) {
        ble_gap_rd_rem_sup_feat_tx(ev->conn_handle);
    } else if (ev->status == 0) {
        ble_gap_event_connect_call(ev->conn_handle, ev->status);
    }
#endif
}

[vlucent]

Have you considered using a more recent version of NimBLE-Arduino? The code in question, as well as the function it resides in, no longer exist in the master branch. This update occurs after the 2.5 release however.