Provide incoming HCI buffer for BTstack

[mringwal]

The current driver reserves a larger buffer for the BTstack Pre-Buffer in memory, but does not use it when receiving an HCI packet from CYW43 driver. As some BTstack events will use this buffer, this causes an out-of-bound write / crash with BTstack v1.6.2 or higher.

Note: the 4-byte CYW43 header implicitly provided a 3-byte pre-buffer, which worked for BTstack v1.6.1 and earlier. In BTstack v1.6.2, the pre-buffer was increased which causes this bug to unhide.

This replaces #2157 (comment)

[peterharperuk]

I changed the branch to develop

[lurch]

I've got no idea why this was assigned to me (I have no BTstack knowledge), so assigning to Peter instead.

[peterharperuk]

Oops - I should get this in. Will just run a quick test.

[peterharperuk]

@kilograham Can we merge this?

[kilograham]

Also i don't understand this at all - maybe a comment about what the various ranges of the buffer are used for (particularly the 4 vs the PREBUFFER - why are they not one and the same, and where alignment comes into play)

[mringwal]

I have improved the documentation and merged the CYW43 packet header buffer with BTstack's pre-buffer, by asserting that HCI_INCOMING_PRE_BUFFER_SIZE is >= CYW43_PACKET_HEADER_SIZE (4).

It's better to read now. However, the build fails for me with "#error HCI_INCOMING_PRE_BUFFER_SIZE not defined or smaller than 4 (CYW43_PACKET_HEADER_SIZE) bytes. Please update btstack_config.h".

@peterharperuk Could you try to find out, where HCI_INCOMING_PRE_BUFFER_SIZE is set?
I get the feeling that different btstack_config.h are used to compile btstack_hci_transport_cyw43.c and the rest of BTstack. Both need to see the same value of HCI_INCOMING_PRE_BUFFER_SIZE to avoid memory issues. E.g. the GATT Client has some requirements on the pre-buffer size, which needs to be provided by btstack_hci_transport_cyw43.c. Feel free to ask for specific details.

[peterharperuk]

Could you try to find out, where HCI_INCOMING_PRE_BUFFER_SIZE is set?

As far as I can see, it's coming from btstack/src/hci.h

// BNEP may uncompress the IP Header by 16 bytes, GATT Client requires six additional bytes for long characteristic reads
// wih service_id + connection_id
#ifndef HCI_INCOMING_PRE_BUFFER_SIZE
#ifdef ENABLE_CLASSIC
#define HCI_INCOMING_PRE_BUFFER_SIZE (16 - HCI_ACL_HEADER_SIZE - 4)
#else
#define HCI_INCOMING_PRE_BUFFER_SIZE 6
#endif
#endif

[mringwal]

@peterharperuk Does it compile for you? Also: where is support for Classic and/or LE defined? If Classic would be defined, then HCI_INCOMING_PRE_BUFFER_SIZE should be (16 - HCI_ACL_HEADER_SIZE - 4) = 8, which is larger than 4. I wasn't able to compile this as it seems to have picked up the value 2 from the #else branch - without Classic.

[peterharperuk]

If you link to pico_btstack_classic it defines ENABLE_CLASSIC and if you link to pico_btstack_ble it defines ENABLE_BLE. Most of the examples are linking to both. https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_btstack/CMakeLists.txt#L107

Let me do some more testing. We haven't updated btstack to v1.6.2, we're still using v1.6.1. Should we update?

[mringwal]

Interesting.

So, how does btstack_hci_transport_cyw43.c know which HCI_INCOMING_PRE_BUFFER_SIZE to use? Is it part of pico_btstack_classic as well as pico_btstack_ble.

If in doubt, we can just use a rather large HCI_INCOMING_PRE_BUFFER_SIZE, e.g. 16, for . btstack_hci_transport_cyw43. We don't plan on increasing the pre-buffer and just did a larger rewrite of the GATT Client logic, which is a big user of HCI_INCOMING_PRE_BUFFER_SIZE.

Yes, you should update to master (1.6.2 + one additional fix).

[peterharperuk]

There's only on version of btstack_hci_transport_cyw43.c. So if you link to classic you get 8. If you ONLY link to ble you get 2. If you link to both you get 8? Is that wrong? I'm fine with us adding a set number to be safe. I thought your change was ok though?

[mringwal]

btstack_hci_transport_cyw43.c should use whatever hci.h choses based on ENABLE_CLASSIC or not.
If that holds true, everything is fine.

(I don't understand the pico-sdk build system. I generally assume that a particular .c file is only compiled once into an .o file (by this having a fixed value for HCI_INCOMING_PRE_BUFFER_SIZE) and then I don't understand how an example can link to pico_btstack_classic as well as pico_btstack_ble at the same time).

[peterharperuk]

Everything is a cmake interface library in the sdk https://cmake.org/cmake/help/latest/command/add_library.html#interface-libraries

[mringwal]

@peterharperuk Thanks. Interesting, I need to revisit my concept of "library". Didn't know it could set defines for the main project (which totally makes sense if you start to thing about it).

Anyway. There's a new force-pushed version. It validates the size of the outgoing buffer and throws an error if needed, as the outgoing buffer is owned by hci.c. For the incoming buffer, which we need to provide, it increases it if needed.

By this, pico-sdk should compile with the current (older) BTstack version as well as newer ones (v1.6.2 will require a larger pre-buffer for LE examples).

[peterharperuk]

Tested with 1.6.1 and 1.6.2 #2202