Fix buffer underflow when receiving BT packets

[ricardoquesada]

When using BTstack 1.6.2 (latest stable version), the microcontroller might crash due to buffer underflow.
The byte before the first byte of hci_packet_with_pre_buffer will get overwritten.

In particular the problem was that BTstack
setup_long_characteristic_value_packet() was receiving &hci_packet_with_pre_buffer[13], and in that function the packet gets overwritten starting from "- LONG_CHARACTERISTIC_VALUE_EVENT_HEADER_SIZE", which is 14. So the byte before hci_packet_with_pre_buffer gets overwritten.

See: https://github.com/bluekitchen/btstack/blob/5d4d8cc7b1d35a90bbd6d5ffd2d3050b2bfc861c/src/ble/gatt_client.c#L1060

This PR follows the same logic implemented in BTstack ESP32 port. See: https://github.com/bluekitchen/btstack/blob/develop/port/esp32/components/btstack/btstack_port_esp32.c#L104

Fixes bluekitchen/btstack#651

[ricardoquesada]

@mringwal when you have the chance, could you review this PR ?

[mringwal]

@ricardoquesada Thanks for this fix. Yes, BTstack requires a few additional bytes before the actual HCI packet. The change follows the logic in other ports (e.g. posix (hci_transport_h4.c) or esp32).

@peterharperuk Are there any alignment requirements for the pointer passed to cyw43_bluetooth_hci_read? if yes, we could align HCI_INCOMING_PACKET_BUFFER_SIZE to that as the buffer itself is already aligned.

(Btw.. a classic case of "how could this have worked before"?)

[peterharperuk]

It think it has to be 4-byte aligned. I don't quite follow this problem or the fix yet.

[mringwal]

It think it has to be 4-byte aligned. I don't quite follow this problem or the fix yet.

Hi Peter. To avoid some memcpy, BTstack asserts the right to use the bytes before the actual HCI packet to setup it's various events - in this case it's the event for GATT Notification or Read events. In any, there needs to be at least HCI_INCOMING_PRE_BUFFER_SIZE buffer bytes before the actual read but we've missed this for the pico-sdk port - there are no additional bytes reserved currently. BTstack recently changes GATT Client to require additional bytes there, which causes an out-of-bounds write (BTstack modifies the bytes before the HCI buffer).

To fix this:

• define an aligned constant, e.g. HCI_INCOMING_PRE_BUFFER_SIZE_ALIGNED as ((HCI_INCOMING_PRE_BUFFER_SIZE + 3) & 0xfffc)
• use this constant instead of HCI_INCOMING_PRE_BUFFER_SIZE in @ricardoquesada 's PR

This can be applied to the current (i.e. older BTstack version) in the pico-sdk. The current v1.6.2 "just" uncovers the issue.

[mringwal]

hm... wait. There's something with offset 3 in the current code that I don't think should be there.
If it's ok for both of you, 'll have a look at it on Monday and provide an updated + tested fix.

[ricardoquesada]

sounds good to me. thanks @mringwal

[mringwal]

@peterharperuk Is this 3-byte offset related to the CYW43 driver hci_transport_cyw43_packet_handler(hci_receive_buffer[3]?

The driver is called with a buffer, but BTstack is handed the data from offset 3 (which corresponds to an implicit 3 byte pre-buffer, and could be the reason it worked with the older version).

In that case, the PR with the aligned pre-buffer size would be fine.

[peterharperuk]

Are you referring to these lines? It looks like a 4 byte offset to me, not 3.

        int err = cyw43_bluetooth_hci_read(hci_packet_with_pre_buffer, sizeof(hci_packet_with_pre_buffer), &len);
        BT_DEBUG("bt in len=%lu err=%d\n", len, err);
        if (err == 0 && len > 0) {
            hci_transport_cyw43_packet_handler(hci_packet_with_pre_buffer[3], hci_packet_with_pre_buffer + 4, len - 4);
            has_work = true;
        } else {
            has_work = false;
        }

My memory is a bit sketchy but I think cyw43 has a 4 byte "header" where the last byte is the packet type. HCI_INCOMING_PRE_BUFFER_SIZE is set by btstack right?

[ricardoquesada]

closing it since it is replaced by this one: #2165

[peterharperuk]

This should be fixed in develop now.