samples: bluetooth: hci_spi: Invalid cmd_hdr and acl_hdr usage

[CorentinDOROTHEE]

Describe the bug
cmd_hdr and acl_hdr local structs were read but never written.
Causing unpredictable errors in bt_tx_thread such as :
"Invalid HCI CMD packet length" in hci.c:2280
or Imprecise data bus errors on nrf52810.

I have pinpointed the issue to samples/bluetooth/hci_spi/src/main.c using SEGGER J-Link probe

To Reproduce
Steps to reproduce the behavior:

1. have an hci_spi hardware setup
2. Add these Kconfig option to hci_spi sample:
   CONFIG_BT_LOG_LEVEL_DBG=y
   CONFIG_BT_DEBUG_LOG=y
   CONFIG_BT_DEBUG_HCI_DRIVER=y
3. build and flash hci_spi for the controller
4. build and flash peripheral_hr for the host
5. See error on controller serial console

Expected behavior
cmd_hdr and acl_hdr local structs should be filled with the SPI data received from the host cpu

Impact
hci_spi is unusable and crash at random spots depending on the host application:

• Advertiser might not start
• Discovering bluetooth services won't work

Environment (please complete the following information):

• OS: Linux x86_64 Kernel : 5.7.6-arch1-1
• Toolchain : Zephyr SDK (0.11.3)
• Commit : Tag v2.3.0

[lochej]

@CorentinDOROTHEE Thank you very much for finding this bug !

In fact, cmd_hdr and acl_hdr are not even initiailized with 0 values.
So you get whatever is in the thread stack ram 🙃

This may be the reason why this bug was not detected before (due to its random occurance nature).

static void bt_tx_thread(void *p1, void *p2, void *p3)
{
	uint8_t header_master[5];
	uint8_t header_slave[5] = { READY_NOW, SANITY_CHECK,
				 0x00, 0x00, 0x00 };
	struct net_buf *buf = NULL;
	struct bt_hci_cmd_hdr cmd_hdr;   /* <=== Not initialized */
	struct bt_hci_acl_hdr acl_hdr;       /*  <=== Not initiailized*/
	int ret;

	ARG_UNUSED(p1);
	ARG_UNUSED(p2);
	ARG_UNUSED(p3);

	(void)memset(txmsg, 0xFF, SPI_MAX_MSG_LEN);

	while (1) {
		[...]

		switch (rxmsg[PACKET_TYPE]) {
		case HCI_CMD:
			buf = bt_buf_get_tx(BT_BUF_CMD, K_NO_WAIT, &rxmsg[1],
					    sizeof(cmd_hdr));
			if (buf) {
				net_buf_add_mem(buf, &rxmsg[4],
						cmd_hdr.param_len);          /* <=== Read here but never written. cmd_hdr.len is random...*/
			} else {
				LOG_ERR("No available command buffers!");
				continue;
			}
			break;
		case HCI_ACL:
			buf = bt_buf_get_tx(BT_BUF_ACL_OUT, K_NO_WAIT,
					    &rxmsg[1], sizeof(acl_hdr));
			if (buf) {
				net_buf_add_mem(buf, &rxmsg[5],
						sys_le16_to_cpu(acl_hdr.len));    /* <=== Read here but never written. acl_hdr.len is random...*/
			} else {
				LOG_ERR("No available ACL buffers!");
				continue;
			}
			break;
		[...]
	}
}

This problem is still present in master.

The fix for this problem should be integrated in both master and V2.3.0 release as backport.

[lochej]

A fix to this problem would be to either correctly fill the cmd_hdr & acl_hdr with spi data
using memcpy from rxmsg spi buffer.

Or simply remove the two structs and get the header length from rxmsg content directly. This could save a bit of the thread stack memory.

[lochej]

Looks like this bug was introduced in this commit:
#23082 caused this issue.
4b622af#diff-01757f00c05ae26127a503a0c08eb4f0

The author deleted the original memcpy calls filling up the headers.

Maybe author and reviewer can help: @Vudentz @carlescufi