Bluetooth: userchan: fix buffer overflow in hci_packet_complete()

[swkim101]

hci_packet_complete(buf, buf_size) should check whether buf_size is enough.
For instance, hci_packet_complete can receive buf with buf_size 1, leading to the buffer overflow in cmd->param_len, which is buf[3]. This can happen when rx_thread() receives two frames in over 512 bytes, and the first frame size is 511. Then, rx_thread() will call hci_packet_complete() with 1.

==5==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000ad81c2 at pc 0x0000005279b3 bp 0x7fffe74f5b70 sp 0x7fffe74f5b68

READ of size 2 at 0x000000ad81c2 thread T6
    #0 0x5279b2  (/root/zephyr.exe+0x5279b2)
    #1 0x4d697d  (/root/zephyr.exe+0x4d697d)
    #2 0x7ffff60e5daa  (/lib/x86_64-linux-gnu/libc.so.6+0x89daa) (BuildId: 2e01923fea4ad9f7fa50fe24e0f3385a45a6cd1c)

0x000000ad81c2 is located 2 bytes to the right of global variable 'rx_thread.frame' defined in '/mnt/hdd1/sungwoo/zephyr-afl/zephyr/drivers/bluetooth/hci/userchan.c' (0xad7fc0) of size 512 SUMMARY: AddressSanitizer: global-buffer-overflow (/root/zephyr.exe+0x5279b2) Thread T6 created by T2 here:
    #0 0x48c17c  (/root/zephyr.exe+0x48c17c)
    #1 0x530192  (/root/zephyr.exe+0x530192)
    #2 0x4dcc22  (/root/zephyr.exe+0x4dcc22)

Thread T2 created by T1 here:
    #0 0x48c17c  (/root/zephyr.exe+0x48c17c)
    #1 0x530192  (/root/zephyr.exe+0x530192)
    #2 0x4dcc22  (/root/zephyr.exe+0x4dcc22)

Thread T1 created by T0 here:
    #0 0x48c17c  (/root/zephyr.exe+0x48c17c)
    #1 0x52f36c  (/root/zephyr.exe+0x52f36c)
    #2 0x5371dc  (/root/zephyr.exe+0x5371dc)
    #3 0x5312a6  (/root/zephyr.exe+0x5312a6)
    #4 0x52ed7b  (/root/zephyr.exe+0x52ed7b)
    #5 0x52eddd  (/root/zephyr.exe+0x52eddd)
    #6 0x7ffff6083c89  (/lib/x86_64-linux-gnu/libc.so.6+0x27c89) (BuildId: 2e01923fea4ad9f7fa50fe24e0f3385a45a6cd1c)

==5==ABORTING

[jhedberg]

Looks like this is fallout from commit 33c922a by @vChavezB. Before support for TCP was added the driver operated on packet based HCI user channel sockets (i.e. entire frames) rather than stream sockets, so you'd never get incomplete packets. The name of the driver still implies that this might be true, even though it doesn't only operate on user channel sockets anymore.

[jhedberg]

@swkim101 the Compliance check is failing and blocking the PR from being merged because of the long lines in the commit message. Are you able to fix that?

[swkim101]

@swkim101 the Compliance check is failing and blocking the PR from being merged because of the long lines in the commit message. Are you able to fix that?

Sure, done.

[vChavezB]

just a suggestion, if you are already comparing the header len in each case in the switch statement, its not needed here anymore
-> buf_len < header_len .

zephyr/drivers/bluetooth/hci/userchan.c

Line 160 in 3d4f83a

if (buf_len < header_len || buf_len - header_len < payload_len) {

[jhedberg]

just a suggestion, if you are already comparing the header len in each case in the switch statement, its not needed here anymore -> buf_len < header_len .

zephyr/drivers/bluetooth/hci/userchan.c

Line 160 in 3d4f83a

if (buf_len < header_len || buf_len - header_len < payload_len) {

That's a good point, i.e. the first part of that check becomes redundant.